-
Notifications
You must be signed in to change notification settings - Fork 10
Restrict File use case #266
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
Restrict File use case #266
Conversation
ekraffmiller
left a comment
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! Just a couple of questions
| * @param {boolean} [restrict] - A boolean value that indicates whether the file should be restricted or unrestricted. | ||
| * @returns {Promise<void>} -This method does not return anything upon successful completion. | ||
| */ | ||
| async execute(fileId: number | string, restrict: boolean): Promise<void> { |
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.
In the UI, the restrict file modal includes an option of changing the default fileAccessRequest value, and adding termsOfAccessForRestrictedFiles. (these values are also in the Terms tab). Do we want to add this as optional params to the restrict method? Or from the frontend, should it be handled by making a separate API call to update Dataset Terms of Use?
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.
Good question, the API is not accepting params and I think it's ok.
A second API call to update Dataset Terms of Use seems reasonable.
We can do that separately while working on the frontend integration. Is there a use case for doing that in Js-dataverse?
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.
ok, we don't have a use case for updating terms of use yet, so yes would be good to work on that separately.
|
@ekraffmiller ready for review again. |
ekraffmiller
left a comment
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, approved!
|
tests are passing - merging pr |
What this PR does / why we need it:
Adds the Restrict File use case. It can restrict or unrestrict a file.
Which issue(s) this PR closes:
Suggestions on how to test this:
Review code and tests.