-
Notifications
You must be signed in to change notification settings - Fork 491
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
IQSS/10251 fix for multifile downloads #10264
Conversation
There was a problem hiding this 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.
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?) |
Could you please refresh the branch - thanks! |
…ultifile_downloads
Looks great. Multi-file downloads are working. |
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: