-
Notifications
You must be signed in to change notification settings - Fork 8
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
Replace File use case #267
Conversation
Updated branch with develop |
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 overall, just one concern
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.
There are something related to pid as well, and we could also add a functional test to it
Hi @ChengShi-1, I addressed all your requested changes. I didn't add functional test for this, there are already unit and integration tests, it's a little bit tricky to add functional tests for this as they depend on s3 bucket direct upload and it will require too much boilerplate for things that are tested already in the integration tests. |
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 to me, approve!
tests are passing - Merging PR |
What this PR does / why we need it:
Adds a new use case to the package: Replace File.
It is focused when a Dataverse installation is configured to use S3 storage with direct upload enabled.
It is based on this API endpoint.
Which issue(s) this PR closes:
Suggestions on how to test this:
Code and Tests review.