-
Notifications
You must be signed in to change notification settings - Fork 488
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/8553 fixrequest access to files in datasets with custom terms #8555
Iqss/8553 fixrequest access to files in datasets with custom terms #8555
Conversation
|
||
logger.fine("Download popup is not required."); | ||
return false; | ||
return null; |
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.
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"
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.
Added a method header comment and a comment at this line.
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.
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.
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 good, passing to QA
thank you |
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: