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/10251 fix for multifile downloads #10264

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Jan 24, 2024

What this PR does / why we need it: Removes ~duplicate code that got out of sync, causing the download bug discussed in the issue. It also addresses a null issue seen in the logs in that issue that may or may not have contributed to the problem.

Which issue(s) this PR closes:

Closes #10251

Special notes for your reviewer: FWIW: As far as I can tell, we've shown the terms popup if the license is not the default since the beginning of the multi-license code. The check against CC0, as far as I've seen is all from when we only had CC0 or custom (and I must have cut/pasted it forward from the pre-multilicence ADA branch when finishing the guestbook-at-request work.)

Suggestions on how to test this: See test case at the end of the issue.

edit: here's the comment describing how to reproduce the issue; super straightforward, should be very easy to test "before" and "after" (L.A.)

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

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

Additional documentation:

@qqmyers qqmyers added the Size: 3 A percentage of a sprint. 2.1 hours. label Jan 24, 2024
@coveralls
Copy link

coveralls commented Jan 24, 2024

Coverage Status

coverage: 20.145% (+0.003%) from 20.142%
when pulling 68f5ed2 on GlobalDataverseCommunityConsortium:IQSS/10251-fix_for_multifile_downloads
into e9215e3 on IQSS:develop.

@landreev landreev self-assigned this Jan 24, 2024
@landreev landreev self-requested a review January 24, 2024 21:48
Copy link
Contributor

@landreev landreev left a comment

Choose a reason for hiding this comment

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

Looks great, thank you so much, Jim!
I was hoping, or even expecting that this would be something simple and easy to fix. Pleasantly surprised by just how simple. 😄
The PR appears to have been fallen victim to a rejection by Jenkins. I just started it by hand - not that there's anything in it that would affect the api tests; but just in case.

@landreev
Copy link
Contributor

The Jenkins run did in fact bomb, but not on account of any failing tests, or anything in this PR, from what I can tell. (died in the installer, it looks like?)

@landreev landreev assigned landreev and unassigned landreev Jan 25, 2024
@landreev
Copy link
Contributor

Could you please refresh the branch - thanks!

@landreev
Copy link
Contributor

Looks great. Multi-file downloads are working.
Confirming that the popup is NOT shown when the dataset is covered by the license that is the default instance-wide. (We are positive this is the correct behavior... - right?)
Merging!

@landreev landreev merged commit 752846f into IQSS:develop Jan 25, 2024
11 checks passed
@landreev landreev removed their assignment Jan 25, 2024
@pdurbin pdurbin added this to the 6.2 milestone Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 3 A percentage of a sprint. 2.1 hours.
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

Multi-file downloads not working in the UI in 6.1 with "CC BY 4.0", possibly other licenses
4 participants