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

Add initial support for .eln file format #9366

Merged
merged 5 commits into from
Feb 9, 2023
Merged

Conversation

NicolasCARPi
Copy link
Contributor

@NicolasCARPi NicolasCARPi commented Feb 3, 2023

What this PR does / why we need it:

This PR adds initial support for .eln files, which are zip archives.

Which issue(s) this PR closes:

It closes not #9363 but it gets the ball rolling. Using yoda speak here to prevent github from closing upon merge.

Special notes for your reviewer:

Given that the file is a zip, I didn't edit MimeTypeDisplay or MimeTypeFacets.

Suggestions on how to test this:

Try and upload a .eln file, as can be found here: https://github.com/TheELNConsortium/TheELNFileFormat/tree/master/examples/elabftw

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

I don't think so.

Is there a release notes update needed for this change?:

Yes.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

@NicolasCARPi thanks for the pull request. Please take a look at the comments I left.

doc/release-notes/9363-eln.md Outdated Show resolved Hide resolved
@pdurbin pdurbin added Feature: File Upload & Handling Size: 3 A percentage of a sprint. 2.1 hours. labels Feb 3, 2023
@pdurbin
Copy link
Member

pdurbin commented Feb 3, 2023

This is a tiny one-line change to detect .eln files as zip files rather than unknown. I'm giving this our smallest size, which is 3.

Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
@mreekie mreekie added this to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation Feb 8, 2023
@NicolasCARPi
Copy link
Contributor Author

NicolasCARPi commented Feb 9, 2023

Hello,

I have updated this PR with the now accepted IANA mime type: https://www.iana.org/assignments/media-types/application/vnd.eln+zip

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Just a comment about how we put files in broad buckets.

src/main/java/propertyFiles/MimeTypeFacets.properties Outdated Show resolved Hide resolved
@pdurbin
Copy link
Member

pdurbin commented Feb 9, 2023

I have updated this PR with the now accepted IANA mime type

@NicolasCARPi wow, that was fast! Thanks for updating the PR. I left a new review with a comment but I'm also ok with moving this to QA for testing.

Something else to think about: right now the file type detection is entirely based on the file extension. This is a good start but in the future we might as if there's a way to detect the file type by inspecting the first few bytes of the file or something. We detect a lot of files this way and only fall back on extensions if we have to.

Thanks again for the PR! Congrats on your new MIME type!

@NicolasCARPi
Copy link
Contributor Author

inspecting the first few bytes of the file or something.

The magic number will be the same as zip (because at the end of the day it's a zip). But a 100% effective way to detect it (on top of the file extension), is to look for the presence of ro-crate-metadata.json file in the archive. This file is guaranteed to be there in all .eln. And it will also be the entrypoint when importing its data, so if the file is not present it might raise an error such as "Expecting ELN file format but could not find metadata file!".

@pdurbin pdurbin moved this from Ready for Review ⏩ to Review 🔎 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Feb 9, 2023
@pdurbin pdurbin self-assigned this Feb 9, 2023
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

This is looking good. Thanks for the pull request and for responding to my feedback, @NicolasCARPi!

This should be a nice stepping stone to other ideas expressed in the linked issue.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Review 🔎 to Ready for QA ⏩ Feb 9, 2023
@pdurbin pdurbin removed their assignment Feb 9, 2023
@kcondon kcondon self-assigned this Feb 9, 2023
@kcondon kcondon merged commit d105308 into IQSS:develop Feb 9, 2023
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Ready for QA ⏩ to Done 🚀 Feb 9, 2023
@NicolasCARPi NicolasCARPi deleted the 9363-eln branch February 9, 2023 22:33
@pdurbin pdurbin added this to the 5.13 milestone Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: File Upload & Handling Size: 3 A percentage of a sprint. 2.1 hours.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

3 participants