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

feat(datasets): user can import datasets though the UI #778

Merged
merged 1 commit into from
Mar 12, 2020

Conversation

vfried
Copy link
Contributor

@vfried vfried commented Feb 21, 2020

Closes #528

There is a new button in the UI so the user can import datasets using the UI.

There are some things to be fixed in the backend for this to properly work.

Button:
image

Form:
image

Deployment: https://virginiatest.dev.renku.ch/

@vfried vfried requested a review from a team as a code owner February 21, 2020 15:14
@rokroskar
Copy link
Member

preview?

@vfried
Copy link
Contributor Author

vfried commented Feb 21, 2020

image

@vfried vfried force-pushed the 528-dataset-import branch 2 times, most recently from 227ee80 to be613d8 Compare February 25, 2020 15:30
@vfried
Copy link
Contributor Author

vfried commented Feb 25, 2020

Updated... and ready to be tested

image

Deployment: https://virginiatest.dev.renku.ch/

@rokroskar
Copy link
Member

What is the URL that this lives on? What I mean is, can I do something like

https://renku/projects/myproject/datasets/import?url=doi://12345

This would make integration with third party pages like dataverse datasets much easier I guess?

@vfried vfried changed the title WIP feat(datasets): user can import datasets though the UI feat(datasets): user can import datasets though the UI Feb 25, 2020
@vfried
Copy link
Contributor Author

vfried commented Feb 25, 2020

What is the URL that this lives on? What I mean is, can I do something like

https://renku/projects/myproject/datasets/import?url=doi://12345

This would make integration with third party pages like dataverse datasets much easier I guess?

At the moment you can't do that but we can make a new issue to allow this to happen, I guess if you where to put this url in the browser you would be redirected to renku, the field would have the doi and you need to press the import button....

@rokroskar
Copy link
Member

Yes! we should investigate what the best flow would be for this, but it would be super useful.

@vfried
Copy link
Contributor Author

vfried commented Feb 25, 2020

Yes! we should investigate what the best flow would be for this, but it would be super useful.

I edited my comment on top...

@ciyer ciyer added this to the sprint-2020-02-13 milestone Feb 26, 2020
@vfried vfried force-pushed the 528-dataset-import branch 3 times, most recently from f72c07f to fdf4dff Compare February 26, 2020 15:58
@ciyer
Copy link
Contributor

ciyer commented Feb 27, 2020

This is a nice feature. We should use it in the tutorial. I like the UI and the code looks great as well.

I have a few bugs that came up in testing:

  1. "Enter" in the URI/DOI field causes the page to reload; I would expect it to submit the form

  2. Put junk into the URI/DOI field, and you get "Please wait, dataset import will start soon" forever

image

  1. If you try to import the same file twice, the server returns with an error, but the UI stays in "Please wait, dataset import will start soon"

image

image

@vfried
Copy link
Contributor Author

vfried commented Feb 27, 2020

Fixed:

  1. The enter was actually a problem on how the form was built, the button had the onClick event instead of the Form having the onSubmit event.

  2. This is a backend problem, i also detected this but i forgot to say to sam. I actually stop the dataset import after a looong time of waiting (the problem is i don't want to just tell the user that dataset import failed too fast)

  3. I also detected this problem and forgot to tell to sam, i am not getting anything in the UI that makes me realise that there is an error... all i get is that the job was ENQUEUED.

image

@jsam
Copy link

jsam commented Mar 5, 2020

I added tests for those cases and things seem to pass there correctly now.

  1. (In tests) This case splits into 2 possible outcomes:
    2.1) requested format is invalid in which case you should receive the following response:
{
   "result":{
      "updated_at":"2020-03-05T17:27:12.431676",
      "extras":{
         "error":"Invalid parameter value - Could not process junkjunkjunk.\nCannot parse URL."
      },
      "job_id":"f938c53eac7b43ed8b2462146cbfa7dd",
      "created_at":"2020-03-05T17:27:12.309677",
      "state":"FAILED"
   }
}

2.2) requested uri could not be resolved (for example invalid doi or url) in which case you should receive the following response:

{
   "result":{
      "created_at":"2020-03-05T17:32:12.233638",
      "updated_at":"2020-03-05T17:32:12.783466",
      "extras":{
         "error":"Invalid parameter value - Could not process 10.5281/zenodo.11111111111111111.\nURI not found."
      },
      "state":"FAILED",
      "job_id":"387d7e7970024b048be0fc272156f888"
   }
}

  1. In this case you should receive response like this:
{
   "result":{
      "created_at":"2020-03-05T17:18:11.089451",
      "updated_at":"2020-03-05T17:18:30.178826",
      "extras":{
         "description":"pybossa-v3.1.2.zip",
         "total_size":714923,
         "progress_size":714923,
         "error":"Dataset exists: \"scifabricpybossa_v31_v312\"."
      },
      "job_id":"1cfc032c7cdd4791881cb8ae1c14ddcb",
      "state":"FAILED"
   }
}

Those error message are coming directly from the core module so they should match the same behavior as we have on the CLI. If you need messaging to be improved we should improve it there.

Let me know if you are not receiving expected results (after we merge the PR).

@vfried
Copy link
Contributor Author

vfried commented Mar 6, 2020

@jsam thanks for the changes, i will test them ASAP!!!

@vfried vfried force-pushed the 528-dataset-import branch 5 times, most recently from 4e88043 to 97676f8 Compare March 11, 2020 08:29
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Nice addition!

I have found a couple of minor issues in the code. Currently, there is an error in the src/project/datasets/DatasetsListView.js file preventing me to open the dataset page in a project

Screenshot_20200311_113409

src/api-client/dataset.js Outdated Show resolved Hide resolved
src/dataset/Dataset.present.js Outdated Show resolved Hide resolved
src/model/RenkuModels.js Outdated Show resolved Hide resolved
@vfried vfried force-pushed the 528-dataset-import branch 2 times, most recently from a9cfc50 to 1a56722 Compare March 11, 2020 13:47
@vfried
Copy link
Contributor Author

vfried commented Mar 11, 2020

@lorenzo-cavazzi i just fixed the lint problems and also the error when entering dataset pages, i think this was related to some other PR, should be fixed now after the rebasing.

Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Great feature! It works well 👍
I have found a couple of minor details (a missing space in text and <Col> sizing) to be adjusted.

I think the DatasetImport.present component has part of the code that should go to the DatasetImport.container since it's about getting data from the backend and not about the presentation of the content. Feel free not to address this if you think it's not needed, but it may help to maintain the componet in the future.

src/model/RenkuModels.js Outdated Show resolved Hide resolved
src/project/Project.present.js Show resolved Hide resolved
src/project/datasets/import/DatasetImport.present.js Outdated Show resolved Hide resolved
src/project/datasets/import/DatasetImport.present.js Outdated Show resolved Hide resolved
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

All good! I am sure this feature will be appreciated by the users

@vfried vfried merged commit edda85d into master Mar 12, 2020
@ciyer ciyer modified the milestones: sprint-2020-02-13, 0.9.0 Mar 24, 2020
@ciyer ciyer deleted the 528-dataset-import branch March 25, 2020 09:51
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.

Import datasets from different sources
5 participants