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

IQSS/9095-dvwebloader integration #9096

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Oct 25, 2022

What this PR does / why we need it: This PR allows an admin to enable and configure a connection with the dvwebloader tool to allow upload of a whole folder tree of files to Dataverse.

Which issue(s) this PR closes:

Closes #9095

Special notes for your reviewer: As with Globus, dvwebloader started as a dataset-level external tool and the limitations there (doesn't appear until one file has been uploaded, appears regardless of whether the dataset uses a store that supports it, isn't in an obvious place for uploads) apply for dvwebloader as well.

FWIW: The dvwebloader is relatively simple at present but will be gaining functionality over time (e.g. implementing more things that the command-line DVUploader does.

Suggestions on how to test this: Follow the docs to configure the dvwebloader and create a dataset using an S3/direct upload store. Verify that upload through the tool works. Regression - verify normal upload is not affected.

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

Is there a release notes update needed for this change?: probably should announce it.

Additional documentation:

IQSS/9126-allow_workflow_tokens_in_access_api
@coveralls
Copy link

coveralls commented Nov 7, 2022

Coverage Status

Coverage decreased (-0.007%) to 20.004% when pulling 78bc5a5 on GlobalDataverseCommunityConsortium:DVWebloader_integration into 6154aac on IQSS:develop.

@mreekie
Copy link

mreekie commented Dec 14, 2022

added to sprint Dec 15, 2022

@pdurbin pdurbin self-assigned this Dec 21, 2022
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I'm leaving some comments in this review.

Before testing, I merged the latest from develop into this PR. The files never showed up in the dataset. I'll try to circle back and troubleshoot some more.

I pushed the merge as well as a small doc update because without asking I wasn't able to figure out how to get the "upload a folder" UI to appear. Before the merge, all API tests we passing, which is good.

Here are screenshots from my testing:

Screen Shot 2022-12-21 at 11 26 21 AM
Screen Shot 2022-12-21 at 11 31 45 AM
Screen Shot 2022-12-21 at 11 26 32 AM
Screen Shot 2022-12-21 at 11 31 53 AM
Screen Shot 2022-12-21 at 11 32 15 AM
Screen Shot 2022-12-21 at 11 32 26 AM

Still no files after a refresh

Screen Shot 2022-12-21 at 11 32 58 AM

doc/sphinx-guides/source/user/dataset-management.rst Outdated Show resolved Hide resolved
/**
* The URL for the DvWebLoader tool (see github.com/gdcc/dvwebloader for details)
*/
WebloaderUrl
Copy link
Member

Choose a reason for hiding this comment

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

As discussed yesterday, we should probably get in the habit of using MPCONFIG, and not just to make @poikilotherm happy. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

If I read the existing code right, the new classes can replace a JVM property but code to replace settings (and keep the ability to dynamically change them) isn't in place. If it is, please point me at an example. (FWIW: #9175 was my first mpconfig jvm option and it appears to work so I'll try to keep adding mpconfig for those).

Copy link
Member

Choose a reason for hiding this comment

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

I see. @poikilotherm I just moved this to QA but if you have ideas, please let us know. Thanks.

doc/release-notes/9096-folder-upload.md Outdated Show resolved Hide resolved
src/main/java/propertyFiles/Bundle.properties Outdated Show resolved Hide resolved
src/main/java/propertyFiles/Bundle.properties Outdated Show resolved Hide resolved
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
@pdurbin
Copy link
Member

pdurbin commented Dec 21, 2022

Still no files after a refresh

I added this to a list of issues here:

@pdurbin
Copy link
Member

pdurbin commented Dec 21, 2022

@qqmyers explained that the reason files weren't appearing in my dataset is that I didn't have CORS enabled on my S3 bucket. The docs on this are in pretty bad shape (documented in the dev guide rather than the installation guide) but I tried to improve them a bit in 78bc5a5.

Here's the working sequence (showing the console to make sure the uploads to S3 are happening):

Upload complete

Screen Shot 2022-12-21 at 3 50 42 PM

No files on dataset page until you refresh

Screen Shot 2022-12-21 at 3 50 47 PM

After clicking refresh, the files are present

Screen Shot 2022-12-21 at 3 51 06 PM

I think this is ready for QA. Great feature.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

As I said in my last comment, I feel this is ready for QA.

@poikilotherm sorry, @qqmyers and I are not sure how to apply MPCONFIG to this one. Please jump in that review comment thread if you have ideas. Thanks.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Ready for Review to QA ✅ Dec 21, 2022
@kcondon kcondon self-assigned this Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GDCC: DataverseNO Size: 10 A percentage of a sprint. 7 hours.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Integrate DVWebloader into the upload panel
5 participants