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/10450 - Fixes for Globus download (transfer out) and related info messages #10451

Merged

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Mar 29, 2024

What this PR does / why we need it: The PR fixes the issue with multifile Globus transfers out/downloads not working for draft datasets. It also improves handling of cases where ineligible files are selected for download or Globus transfer by only showing the download/transfer mechanisms that will work (on some files) given the files in the dataset and by improving the UI messages to indicate that files may not be eligible either because the user doesn't have permission (restricted or embargoed), or because the files can't be downloaded/transferred (i.e. files not in a Globus store when the user tries a Globus transfer or files in a Globus store that doesn't support normal downloads when the user selects download.)

The PR also includes a fix for a problem doing a second download w/o refreshing the page. The underlying issue was that a const (newWin) was used when opening the app window as part of checking for pop-up blockers and reuse of that constant when the script runs a second time causes a failure (visible in the browser console). Using a ~unique name for the constant avoids the issue. (FWIW: The same code is run for external tool launching, where the problem isn't seen. I assume something in the ajax refreshing is different between the two cases.)

Which issue(s) this PR closes:

Closes #10450

Special notes for your reviewer: The bug will affect Globus downloads in the current develop/6.2 candidate. W.r.t. messaging, this PR does some cleanup so that the popup dialogs make more sense when Globus is used. FWIW: PR #10336 is ready for QA and will add another potential case (files that are not accessible due to their retention period having expired.) For/after that PR, we could consider trying to simplify the messaging, and or highlighting which files are ineligible for which reason. (Moving the logic to Java might make it easier to handle the mixed cases versus trying to keep the current logic with JSF ? operators to choose between fixed bundle strings.)

Suggestions on how to test this: Per the issue, create a dataset with a mix of Globus and non-globus files and verify a) that Globus transfer from a draft dataset works (nominally that Dataverse attempts to launch the Dataverse-Globus app for this case), and b) that selecting either download Original or Globus Transfer from the Access menu or file table Download menu shows correct dialog messages, and c) that if there are no downloadable files in the dataset (only Globus transferrable ones) that the download Original/Tabular options don't appear.

Try a second download attempt on the same page (same or different set of files) - it should work. (Regression: external tool launching should continue to work.)

Does this PR introduce a user interface change? If mockups are available, please link/include them here: Only changes strings which are readable in the Bundle file.

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

Additional documentation:

@qqmyers qqmyers added the Size: 10 A percentage of a sprint. 7 hours. label Mar 29, 2024
@lubitchv
Copy link
Contributor

lubitchv commented Apr 4, 2024

I tested this branch on my local machine and it seems to work.

dataset.mixedSelectedFilesForTransfer=Some file(s) cannot be transferred. (They are restricted, embargoed, or not Globus accessible.)
dataset.inValidSelectedFilesForTransfer=Ineligible Files Selected
dataset.downloadUnrestricted=Click Continue to download the files you have access to download.
dataset.transferUnrestricted=Click Continue to transfer the elligible files.

dataset.requestAccessToRestrictedFiles=You may request access to the restricted file(s) by clicking the Request Access button.
dataset.requestAccessToRestrictedFiles=You may request access any restricted file(s) by clicking the Request Access button.
Copy link
Contributor

Choose a reason for hiding this comment

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

It should probably be "to any" here

Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

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

Thanks for the quick change. The rest looks reasonable.

@sekmiller sekmiller removed their assignment Apr 11, 2024
@landreev landreev self-assigned this Apr 16, 2024
@landreev
Copy link
Contributor

Unfortunately, the downloads are still not working me; either with the Demo/POC or the Prod. NESE tape stores configured. The behavior and the errors in the Globus online app activity views are identical to what I posted on slack last week, but I will add the screenshots here.
But let me re-test everything very carefully one more time.

@landreev
Copy link
Contributor

Further testing indicated that the failure is specifically within the Globus app; so I will work with Victoria to resolve that.

@landreev
Copy link
Contributor

With the bug in the Borealis app identified, I am back to testing functionality in the PR. Will try to wrap up the QA asap.

@landreev
Copy link
Contributor

One thing that I should have tried much sooner - attempting a Globus download of a public file, as a guest user; I realized that globusDownloadParameters requires an AuthenticatedUser api token. Just want to confirm that this is how it was meant to be implemented. And if so - meaning that public Globus-stored files cannot be downloaded by guest users via the app - should the permission logic on the dataset page be changed accordingly?

a quick experiment to see if changes elsewhere may be needed
@landreev
Copy link
Contributor

A simple code change that allows null authUser in the api method appears to be sufficient; all the code further down is still working - a null api token is issued and the utility that outputs the urls for api calls leaves them unsigned.
I got the full download workflow to work successfully a few times in the Globus app as well.

@landreev
Copy link
Contributor

I keep however seeing some intermittent errors (unrelated to guest vs. auth. user, I'm fairly positive) where the download fails and(/or) the Globus collection permission is left in place. The latter condition then consistently prevents new downloads from starting with ACL already exists or Endpoint ACL already has the maximum number of access rules in the server log.

I'm trying to gauge if this is something we want to try to diagnose as part of this pr - ?

@landreev
Copy link
Contributor

I checked in another quick fix for a (less fatal but messy) error at the end of a Globus download by a guest user. Otherwise, it seems prudent to keep the PR within its intended scope and merge it.
The entire workflow appears to still be a bit flaky. There will likely be complaints and issues with it once it's in use, but we'll have to deal with them separately.

@landreev
Copy link
Contributor

I just synced the branch with (the main) develop. Mostly because I saw that the annoying harvesting test (that I believe should have been fixed in develop) failed in Jenkins the other day on the pr.

@coveralls
Copy link

Coverage Status

coverage: 20.712% (-0.005%) from 20.717%
when pulling af4f918 on GlobalDataverseCommunityConsortium:GlobusDownload
into 447d576 on IQSS:develop.

@landreev
Copy link
Contributor

Tests passed, merging!

@landreev landreev merged commit 516a73f into IQSS:develop Apr 19, 2024
11 checks passed
@landreev landreev removed their assignment May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GREI 5 Use Cases Size: 10 A percentage of a sprint. 7 hours.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Globus Multifile Download Broken for Draft datasets, confusing messages
7 participants