-
Notifications
You must be signed in to change notification settings - Fork 16
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 integrate file upload page with use cases (early implementation) #418
Feature/417 integrate file upload page with use cases (early implementation) #418
Conversation
I am still working on it. This PR is created as an early demo to show where it is going. I think that some of the things that still need more work and are listed in the followup issues are out of the scope of this PR. The things I will still do within this PR:
|
I updated #431 to keep track of the remaining work. |
Approving! Ready for QA. 👍🏼 |
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, how have you tested the integration with S3 storage? That is, the end-to-end functionality, without mocks, but with a real S3 storage. Have you used a remote S3 storage?
It is necessary to implement some end-to-end (e2e) tests along with the integration. You can see other examples of e2e tests in the repository.
We need to add S3 storage to the containerized environment to be able to test the functionality locally. This is also necessary for the e2e tests (since they use the containerized environment). For this, we can reuse the docker-compose configuration I added to js-dataverse https://github.com/IQSS/dataverse-client-javascript/blob/develop/test/environment/docker-compose.yml.
I can help you set up the containers, but first I want to confirm that you've managed to get the integration working, as I haven't been able to get it to work yet with the docker-compose configuration I mentioned.
Since there was still a lot of functionality missing, I focused in this PR on adding an uploaded files list, buttons, modals, etc. It already got very big. I did split up the remaining work in smaller peaces. I think before writing the integration tests, the interface of upload file needs to be finalized first, so I created this issue: Most weird thing is the add uploaded file expects the file to be passed again? The API it supposed to call looks very differently from that. I also do not see the hashes of the files in there? Are they calculated while the files are uploaded? I thought that js-dataverse was stateless? I can pick it up next and have deeper look at what is happening there, but I think that both calls would need some tweaking before having a full end-to-end functionality. Once that part is finalized, we can write the integration tests: The overview of the remaining work can be found here: #431 In the short run, I can make the current js-dataverse implementation work by ignoring the uploaded files list and letting the files be added directly after upload. |
@GPortas , I implemented a workaround to better fit the current implementation in js-dataverse. Does it work for you now? |
Yes, you are right that it is unnecessary to send the file again to In But actually, this object can be pre-created and sent to the use case by the caller (the SPA in this case). Along with the rest of the enhancements that need to be added to the use case, I will also improve this part. So I am going to merge this PR, but I will not close the associated issue yet, as the integration is still uncompleted. Thank you! |
What this PR does / why we need it:
Integrates the file upload page UI from #366 with the js-dataverse use cases implemented on PR-153.
Which issue(s) this PR closes:
Special notes for your reviewer:
Follow-up issue to this PR: #431
Suggestions on how to test this:
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: