-
Notifications
You must be signed in to change notification settings - Fork 18
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
Feature/417 File upload integration #464
Conversation
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.
Hi @ErykKul great work! just some comments.
Also one thing I noticed in the UI, the checkboxes to select an uploaded file from the UploadedFiles list are not being checked/unchecked on the first click, somehow they only are checked/unchecked on the second click.
I have added the gap. I think that the header warning is something that the js-client sets. I can look at it, as well as improving the progress updates (I think that one would also require changes at the js-client). |
@ErykKul Thanks for addressing the changes. I have retested the feature for .java files and it works great. I think this is ready to be merged, and open a separate issue in js-dataverse to review the header warning and progress updates. @g-saracca |
I tried fixing the header and progress bar issue: IQSS/dataverse-client-javascript#187 |
I think it fixed it, I will add it to this PR, we can revert it if needed. |
…ar and content header error
The progress bar moves nicely and there are no errors in the log. However, multipart upload does not work as it should, it looks like it "resets" with every part (starts from 0 and never reaches 100%). I will try fixing it, it should be easy to do (I think that my assumption on the index and part size were incorrect, or I messed up the formula). |
I think that it was a correct behavior: resetting was due to the upload failing, from the log it looks like uploading first part always failed with some retries (maybe the file is too large for the test-bucket)? Anyway, it is the best I could do for now. I updated the dependency again, after a small fix on the multipart progress updates. I will stop committing now, such that the review and QA can continue. Thanks @g-saracca and @GPortas |
@ErykKul you made changes to the file upload use case on the js-dataverse repo? |
If that is the case we should first I think , review and approve the JS-dataverse PR 👍🏼 |
Yes, it was a small change in the js-dataverse. It is not urgent, I think it is most important to get it right, so I think it is a good idea to take a look at my change at the js-dataverse first: PR 187 |
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.
This works great now. I just merged the js-dataverse PR and tested it on this branch. All issues are solved.
testjsdv.mov
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.
Great job @ErykKul ! approving again and merging!
What this PR does / why we need it:
File upload works now, integration test is added
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Upload some file to a dataset.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No interface changes.
Is there a release notes update needed for this change?:
Something like "added files upload feature"
Additional documentation: