Skip to content
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

Merged
merged 99 commits into from
Jul 24, 2024

Conversation

ErykKul
Copy link
Contributor

@ErykKul ErykKul commented Jun 17, 2024

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:

@ErykKul
Copy link
Contributor Author

ErykKul commented Jun 17, 2024

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:

  • debugging, making sure everything works as intended
  • fixing tests and improving coverage
  • using translations for buttons and other text
  • better analysis of the remaining work and creation of the followup issues

@coveralls
Copy link

Coverage Status

coverage: 98.403% (+0.9%) from 97.547%
when pulling 53e6bca on feature/417-integrate-file-upload-page-with-use-cases
into fa7497a on develop.

@coveralls
Copy link

Coverage Status

coverage: 98.403% (+0.9%) from 97.547%
when pulling 3159a8b on feature/417-integrate-file-upload-page-with-use-cases
into fa7497a on develop.

@coveralls
Copy link

Coverage Status

coverage: 98.403% (+0.9%) from 97.547%
when pulling 5831706 on feature/417-integrate-file-upload-page-with-use-cases
into fa7497a on develop.

@coveralls
Copy link

Coverage Status

coverage: 98.403% (+0.9%) from 97.547%
when pulling 5831706 on feature/417-integrate-file-upload-page-with-use-cases
into fa7497a on develop.

@coveralls
Copy link

Coverage Status

coverage: 97.548% (+0.001%) from 97.547%
when pulling ab49fe0 on feature/417-integrate-file-upload-page-with-use-cases
into fa7497a on develop.

@coveralls
Copy link

Coverage Status

coverage: 97.581% (+0.03%) from 97.547%
when pulling ab49fe0 on feature/417-integrate-file-upload-page-with-use-cases
into fa7497a on develop.

@coveralls
Copy link

Coverage Status

coverage: 98.403% (+0.9%) from 97.547%
when pulling b5d04fb on feature/417-integrate-file-upload-page-with-use-cases
into fa7497a on develop.

@coveralls
Copy link

Coverage Status

coverage: 98.403% (+0.9%) from 97.547%
when pulling 5228ce9 on feature/417-integrate-file-upload-page-with-use-cases
into fa7497a on develop.

@coveralls
Copy link

Coverage Status

coverage: 98.403% (+0.9%) from 97.547%
when pulling 1d163e7 on feature/417-integrate-file-upload-page-with-use-cases
into fa7497a on develop.

@coveralls
Copy link

coveralls commented Jul 16, 2024

Coverage Status

coverage: 97.663% (-0.02%) from 97.682%
when pulling 7d889e4 on feature/417-integrate-file-upload-page-with-use-cases
into 91b2db6 on develop.

@ErykKul
Copy link
Contributor Author

ErykKul commented Jul 16, 2024

I updated #431 to keep track of the remaining work.

@ErykKul ErykKul assigned GermanSaracca and unassigned ErykKul Jul 16, 2024
@ErykKul ErykKul assigned GermanSaracca and unassigned ErykKul Jul 17, 2024
@GermanSaracca
Copy link
Contributor

Approving! Ready for QA. 👍🏼

GermanSaracca
GermanSaracca previously approved these changes Jul 17, 2024
@GermanSaracca GermanSaracca removed their assignment Jul 17, 2024
@GPortas GPortas self-assigned this Jul 18, 2024
Copy link
Contributor

@GPortas GPortas left a 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.

@ErykKul
Copy link
Contributor Author

ErykKul commented Jul 20, 2024

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:
IQSS/dataverse-client-javascript#163

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:
#441

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.

@ErykKul
Copy link
Contributor Author

ErykKul commented Jul 20, 2024

@GPortas ,

I implemented a workaround to better fit the current implementation in js-dataverse. Does it work for you now?

@ErykKul ErykKul removed their assignment Jul 20, 2024
@GPortas GPortas changed the title Feature/417 integrate file upload page with use cases Feature/417 integrate file upload page with use cases (early implementation) Jul 24, 2024
@GPortas
Copy link
Contributor

GPortas commented Jul 24, 2024

@ErykKul

Yes, you are right that it is unnecessary to send the file again to AddUploadedFileToDataset use case.

In AddUploadedFileToDataset, the file blob is only used to extract and calculate properties to build the UploadedFileDTO object that is sent to the API.

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!

@GPortas GPortas merged commit 4d27f8b into develop Jul 24, 2024
16 of 20 checks passed
@GPortas GPortas deleted the feature/417-integrate-file-upload-page-with-use-cases branch July 24, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GREI Re-arch GREI re-architecture-related integration Tasks involving the connection and interaction of UI features with the Dataverse API Size: 10 A percentage of a sprint. 7 hours. SPA: File Upload Page
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

None yet

5 participants