Iqss/8553 fixrequest access to files in datasets with custom terms#8555
Merged
kcondon merged 4 commits intoIQSS:developfrom Mar 31, 2022
Conversation
sekmiller
reviewed
Mar 30, 2022
sekmiller
reviewed
Mar 30, 2022
sekmiller
reviewed
Mar 30, 2022
|
|
||
| logger.fine("Download popup is not required."); | ||
| return false; | ||
| return null; |
Contributor
There was a problem hiding this comment.
Found the use of null here a little confusing, but looking at the rest of the code I see that it means "leave it up to Guestbook if applicable"
Member
Author
There was a problem hiding this comment.
Added a method header comment and a comment at this line.
sekmiller
requested changes
Mar 30, 2022
Contributor
sekmiller
left a comment
sekmiller
left a comment
There was a problem hiding this comment.
A couple of minor issues with the logging and maybe a comment on the meaning of null would be helpful. Overall makes sense and should fix the underlying issue.
sekmiller
approved these changes
Mar 30, 2022
sekmiller
approved these changes
Mar 30, 2022
Contributor
sekmiller
left a comment
sekmiller
left a comment
There was a problem hiding this comment.
Looks good, passing to QA
|
thank you |
qqmyers
deleted the
IQSS/8553-fixrequest_access_to_files_in_datasets_with_custom_terms
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it: The request access popup logic failed with a null pointer if license=null-> custom terms were used. This issue was caught/fixed for the download popup but missed here. The PR includes a refactor so that both methods now use common code.
Which issue(s) this PR closes:
Closes #8553
Special notes for your reviewer: As far as I could tell, prior to the multi-license code, the isDownloadPopupRequired and isRequestAccessPopupRequired methods used the same code except for isDownload doing a final check for guestbooks. With multi-license it looks like they were changed independently at different times (with isRequestAccess being broken). They are now the same again (except the final guestbook check). If there was some other intended change between the two of them, this PR would be removing it.
Is this a basic enough issue to push a 5.10.1 or 5.11 in the short term?
Suggestions on how to test this: Use custom terms on some dataset, add a restricted file(s), publish, login as someone else and verify that you get the request access popup. Could also check for regression (does the download popup appear as before), does everything still work with a standard license, ...). FWIW: There are tests in FileUtil for isDownloadPopupRequired that are passing as before.
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: