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

feat(website): support excel metadata files in data upload form #3469

Merged
merged 47 commits into from
Jan 13, 2025

Conversation

fhennig
Copy link
Contributor

@fhennig fhennig commented Dec 18, 2024

resolves #3334

preview URL: https://excel-upload.loculus.org

Summary

  • throws an error if the sheet is empty
  • shows a warning if there are multiple sheets in the excel file
  • the cell type is encoded in the sheet, so all dates are passed correctly regardless of format.
  • the original file handle is kept around and if the original file changes, the file is unselected.

Screenshot

image

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by an appropriate test.
    • testfiles are added and parsed to check if dates are correct etc.

@fhennig fhennig added the preview Triggers a deployment to argocd label Dec 18, 2024
@fhennig fhennig self-assigned this Dec 18, 2024
@fhennig
Copy link
Contributor Author

fhennig commented Dec 18, 2024

TODO: don't apply the processing for every UploadComponent (currently it also does it for the fasta upload)

@fhennig
Copy link
Contributor Author

fhennig commented Dec 19, 2024

TODO: Testing

  • Test multiple excel versions
  • Test number formats (localized?)
  • test date formats

Copy link
Member

@chaoran-chen chaoran-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Felix, this looks great to me! I tested it with for WNV using data that I just uploaded to #3469. Below are a few minor comments.

@fhennig fhennig requested a review from chaoran-chen January 7, 2025 11:58
@theosanderson
Copy link
Member

The preview seems messed up at the moment - I can't log in (I'm sure that's not to do with the changes you've made just some weird state thing, or pre-existing bug)

@fhennig
Copy link
Contributor Author

fhennig commented Jan 7, 2025

I made an empty commit to try and trigger a redeployment of the preview, I also see the error you're seeing. I can' imagine that the changes I have made are somehow responsible.

@theosanderson
Copy link
Member

When you rebase it should be fixed :)

@fhennig
Copy link
Contributor Author

fhennig commented Jan 8, 2025

I rebased, it works!

I'm getting started on the compression handling, I found a lib, looks like it should be easy to implement

@fhennig
Copy link
Contributor Author

fhennig commented Jan 9, 2025

handling compression is in fact not easy, it was a pain. but I think it should work now, the tests passed locally.

@fhennig
Copy link
Contributor Author

fhennig commented Jan 9, 2025

the lzma lib needs to be swapped out as well.

@theosanderson
Copy link
Member

Sorry for the pain - yes in fairness I'd forgotten how many different methods we support :(

@fhennig
Copy link
Contributor Author

fhennig commented Jan 9, 2025

no worries 😄

I thought "surely there's a a just unpack it lib" - I have used commandline tools that do that before. But alas, I couldn't find one.

And then some of the libs are quite old, I had to try out a few different ones before finding ones that work well and have type support etc. But now it looks like it's working! (EDIT: Shit, test failed again)

@fhennig
Copy link
Contributor Author

fhennig commented Jan 9, 2025

Ok, so this library that I used for xz (xz-decompress) also has some issues, because vite thinks it's commonJS and it's wonky to import.

Now I also found this: https://www.npmjs.com/package/libarchive.js but it requires a custom init call and putting a wasm module somewhere.

I have to continue this tomorrow. Why are all the xz/lzma libs so weird to use? 😢


Alternatively, we just defer this (who would upload xz compressed excel files?!) and just add a popup that that file combination doesn't work yet and that's that. I don't feel like it's really worth spending a lot of time on this.

@theosanderson
Copy link
Member

Alternatively, we just defer this (who would upload xz compressed excel files?!) and just add a popup that that file combination doesn't work yet and that's that. I don't feel like it's really worth spending a lot of time on this.

👍

@chaoran-chen
Copy link
Member

Alternatively, we just defer this (who would upload xz compressed excel files?!) and just add a popup that that file combination doesn't work yet and that's that. I don't feel like it's really worth spending a lot of time on this.

Agree, this sounds good to me!

@fhennig fhennig requested a review from chaoran-chen January 13, 2025 13:18
Copy link
Member

@chaoran-chen chaoran-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, works for me!

@fhennig fhennig merged commit 0f3931e into main Jan 13, 2025
20 checks passed
@fhennig fhennig deleted the excel-upload branch January 13, 2025 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept Excel files for metadata
3 participants