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

238 - Integration single file download #240

Merged
merged 7 commits into from
Dec 12, 2023

Conversation

MellyGray
Copy link
Contributor

@MellyGray MellyGray commented Nov 28, 2023

What this PR does / why we need it:

This pull request establishes a connection between the file download buttons in the files table rows and the access API for downloading files.

Which issue(s) this PR closes:

Special notes for your reviewer:

The integration of files tabular data is currently under review:

As a result, the tabular files' download options won't appear identical to those in the JSF frontend until the aforementioned pull request is merged. Ideally, PR #227 should be merged into the develop branch before this one so that the tabular download options align with the JSF frontend.

Suggestions on how to test this:

Step 1: Run the development environment

  1. Run npm I
  2. cd packages/design-system && npm run build
  3. cd ../../
  4. Check that you have a .env file such as the .env.example, with the VITE_DATAVERSE_BACKEND_URL=http://localhost:8000 variable
  5. cd dev-env
  6. ./run-env.sh unstable
  7. To check the environment go to http://localhost:8000 and check your local dataverse installation

Step 2: Test Dataset View mode with the implemented changes to download the files

  1. Go to http://localhost:8000
  2. Login as admin using username: dataverseAdmin and password: admin1
  3. Create a new Dataset
  4. Upload some files to the dataset
  5. From the dataset view mode copy the search parameters from the url. Ex.: ?persistentId=doi:10.5072/FK2/LHGRHP&version=DRAFT
  6. Go to http://localhost:8000/spa/datasets and paste the search parameters. Ex.: http://localhost:8000/spa/datasets?persistentId=doi:10.5072/FK2/LHGRHP&version=DRAFT
  7. You are currently viewing the same dataset in the SPA. In the files table, locate the "Access File" dropdown on the right of each row. From the dropdown, select the option to download the file and verify that it is successfully downloaded.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

No

Is there a release notes update needed for this change?:

No

Additional documentation:

@MellyGray MellyGray linked an issue Nov 28, 2023 that may be closed by this pull request
@MellyGray MellyGray marked this pull request as ready for review November 28, 2023 17:00
@MellyGray MellyGray added this to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation Nov 28, 2023
@MellyGray MellyGray added the Size: 3 A percentage of a sprint. 2.1 hours. label Nov 29, 2023
@ekraffmiller ekraffmiller self-assigned this Dec 4, 2023
@MellyGray MellyGray added the integration Tasks involving the connection and interaction of UI features with the Dataverse API label Dec 4, 2023
@coveralls
Copy link

coveralls commented Dec 5, 2023

Coverage Status

coverage: 98.295% (+0.003%) from 98.292%
when pulling 6610d27 on feature/238-integration-single-file-download
into 333c7c6 on develop.

@GPortas
Copy link
Contributor

GPortas commented Dec 11, 2023

@MellyGray Please, can you resolve conflicts?

@MellyGray
Copy link
Contributor Author

MellyGray commented Dec 11, 2023

@GPortas conflicts resolved, there's an e2e test failing but it's the one that it's going to be fixed with

@MellyGray MellyGray removed their assignment Dec 11, 2023
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from In Review 🔎 to Ready for QA ⏩ Dec 11, 2023
Copy link
Contributor

@GPortas GPortas left a comment

Choose a reason for hiding this comment

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

Looks good!

downloadsinglefile

@GPortas
Copy link
Contributor

GPortas commented Dec 11, 2023

@MellyGray

Tabular files work well for both original and archival formats. But when selecting RData the following error appears:

{"status":"ERROR","code":404,"message":"datafile access error: requested optional service (image scaling, format conversion, etc.) could not be performed on this datafile."}

Here is a recording:

tabular.mov

@MellyGray
Copy link
Contributor Author

@MellyGray

Tabular files work well for both original and archival formats. But when selecting RData the following error appears:

{"status":"ERROR","code":404,"message":"datafile access error: requested optional service (image scaling, format conversion, etc.) could not be performed on this datafile."}

@GPortas The same error appears in JSF when trying to download a tabular file in RData format.

I found this in the documentation:

The Dataverse Software uses R to handle tabular data files. The instructions below describe a minimal R Project installation. It will allow you to ingest R (.RData) files as tabular data and to export tabular data as .RData files. R can be considered an optional component, meaning that if you don’t have R installed, you will still be able to run and use the Dataverse Software - but the functionality specific to tabular data mentioned above will not be available to your users.

I think we need to install R in the dev-env if we want to test that feature, although I don't think it's necessary since if the users install that option, it should work as expected

@MellyGray MellyGray removed their assignment Dec 11, 2023
@GPortas
Copy link
Contributor

GPortas commented Dec 12, 2023

@MellyGray In my opinion the download option in R format should not be rendered if R is not configured in Dataverse. To do this we could use a public setting that allows the SPA to know if R is configured in the Dataverse instance.

It certainly happens that when testing JSF on my localhost, I see that the R option is available, although clicking it does nothing. I think this behavior should not be expected for the user. Even though JSF handles it this way, I think the SPA should use the approach I described above (conditional rendering based on a public setting).

Maybe @scolapasta, @jggautier or @pdurbin can share their thoughts on this and if there is a reason that explains why it is done that way in JSF.

Copy link
Contributor

@GPortas GPortas left a comment

Choose a reason for hiding this comment

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

By the way, I think the conditional rendering solution I described above should not block the PR at all. Whatever solution we propose, I would implement it in another issue.

For this PR I think we should catch the error that occurs (black screen) when clicking the R option, so that it does nothing (like in JSF) but doesn't break. @MellyGray

@MellyGray
Copy link
Contributor Author

MellyGray commented Dec 12, 2023

By the way, I think the conditional rendering solution I described above should not block the PR at all. Whatever solution we propose, I would implement it in another issue.

For this PR I think we should catch the error that occurs (black screen) when clicking the R option, so that it does nothing (like in JSF) but doesn't break. @MellyGray

@GPortas I can't handle the error since it's an href. Besides, the same black screen behavior is happening in JSF, so I think it should be okay like this until we decide to implement hiding the RData option if R is not installed.

Grabacion.de.pantalla.2023-12-12.a.las.14.08.49.mov

Extensive explanation:

The way the download buttons are working now is that the frontend only adds an href property to the buttons, and the API is the one triggering the browser download because of the headers. So, if the API response doesn't contain the headers to trigger the download, the hyperlink works as a normal hyperlink and navigates the user to that API URI (black screen error). When I was reading the API docs there was something about 'errors to be downloaded as a file' so maybe the API should trigger the browser download and download the error message

@MellyGray MellyGray removed their assignment Dec 12, 2023
@GPortas GPortas self-requested a review December 12, 2023 14:00
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Ready for QA ⏩ Dec 12, 2023
@pdurbin
Copy link
Member

pdurbin commented Dec 12, 2023

In my opinion the download option in R format should not be rendered if R is not configured in Dataverse.

Yes, @BPeuch and probably others agree with you. That's almost exactly how he worded it:

@ekraffmiller ekraffmiller removed their assignment Dec 12, 2023
@GPortas GPortas merged commit 38ceeea into develop Dec 12, 2023
9 of 12 checks passed
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Dec 12, 2023
@GPortas GPortas deleted the feature/238-integration-single-file-download branch December 12, 2023 14:15
jayanthkomarraju pushed a commit to jayanthkomarraju/dataverse-frontend that referenced this pull request May 31, 2024
…file-download

238 - Integration single file download
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration Tasks involving the connection and interaction of UI features with the Dataverse API Size: 3 A percentage of a sprint. 2.1 hours.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Single file download
5 participants