Remove editVersion calls, standardize restricted file render logic #8902
Conversation
This file contains 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
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: Possibly related to #8845 - use of DatasetPage.dataset.editVersion below in situations where a draft does not exist could dynamically create a new draft. Since the uses here (ui:removed on line 299, but active on line 310 and 334) are just intended to check for restricted files for rendering purposes, it doesn't seem like they should ever result in a new draft on purpose. This PR removes those. Further, in looking through the file, it appears there are several other variations in how the check for the existence of restricted files is being done that all nominally result in the same yes/no answer. I've gone ahead and changed all of those to use the same cached DatasetPage.hasRestrictedFiles parameter for clarity.
Which issue(s) this PR closes:
Closes #8845
Special notes for your reviewer: Some discussion on slack dv-core 8/3/22 but no real decision on priority. Seems like a good follow-up for the next release though.
Suggestions on how to test this: Basically just regression testing of the dataset page license/terms functionality, specifically w.r.t. whether items related to restricted files appear/don't appear or are enabled/disabled the same way as in the current version. Bonus points if you find a way in the pre-PR version to cause a ghost-draft-version to be created (one reason for the PR is to prevent that but no one has seen this effect in practice afaik.)
Does this PR introduce a user interface change? If mockups are available, please link/include them here: explicitly no - should not change expected behavior.
Is there a release notes update needed for this change?:
Additional documentation: