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

fix importing geojson files on firefox on linux (#9912) #9913

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

landryb
Copy link
Collaborator

@landryb landryb commented Jan 25, 2024

for some reason mimetype is detected as application/geo+json

Description

cf #9912

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix

Issue

What is the current behavior?

#9912

What is the new behavior?

importing geojson files in the 'add data' & 'import data' item of profile tool passes mimetype check

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • No

for some reason mimetype is detected as application/geo+json
@landryb
Copy link
Collaborator Author

landryb commented Jan 25, 2024

something is still wrong nevertheless, because trying various geojson files in the profile tool import line item, i mostly get The provided line is outside the profile coverage error messages - while they are on the DEM coverage, when reprojected.

@tdipisa tdipisa requested a review from MV88 January 25, 2024 13:41
@tdipisa tdipisa linked an issue Jan 25, 2024 that may be closed by this pull request
1 task
@tdipisa tdipisa added the bug label Jan 25, 2024
@tdipisa tdipisa added this to the 2024.01.00 milestone Jan 25, 2024
@tdipisa
Copy link
Member

tdipisa commented Jan 25, 2024

Hi @landryb, thank you so much for your contribution. Is it possible for you to check failing unit tests?

@landryb
Copy link
Collaborator Author

landryb commented Jan 25, 2024

Hi @landryb, thank you so much for your contribution. Is it possible for you to check failing unit tests?

well i've seen the failure and it is in some cesium-related code, and the error message {"message":"Couldn't find a repository matching this job.","error":true} doesnt mean much to me, so i doubt its actually related to the PR. i dunno if one can retrigger the github action to check..

@landryb
Copy link
Collaborator Author

landryb commented Jan 25, 2024

something is still wrong nevertheless, because trying various geojson files in the profile tool import line item, i mostly get The provided line is outside the profile coverage error messages - while they are on the DEM coverage, when reprojected.

for this, cf #9914

Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

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

Hello @landryb
i'll report here my findings:

Actually there are some issues even before reaching longitudinal profile.
in particular we do not yet support import of geojson files that are not reprojected to EPSG:4326.

see here, trying to import test2154.json on a map with is already using EPSG:2154
image
the bbox is assuming to be in 4326 here

crs: "EPSG:4326"

also because here the projection is expected to be in map.projection which something related to mapstore map.json files

in fact the UI shows this warning
image

Please try to convert the json file to EPSG:4326 and re-test.

Aside from that I have tested using master branch importing a file.geojson or file.json and they both are recognized as application/json so if the geojson is valid (only 4326) I'm able to import it inside the longitudinalProfile tool

i'm using this for testing (remove .txt) in this context
a geosjon line - FC.geojson.txt

i think this PR is not needed tbh

@landryb
Copy link
Collaborator Author

landryb commented Feb 19, 2024

@MV88 did you try with firefox on linux ? i get your comments are about the geojson projection, while this PR/issue isnt about this topic, but about the detected mimetype..

@tdipisa tdipisa modified the milestones: 2024.01.00, 2024.01.01 Mar 4, 2024
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

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

@landryb hi, sorry for being late
I had the time to better review this and indeed .geojson are recognized as geo-json mime type, but in order to better support it you need also to include changes here for the normal map import tool

https://github.com/geosolutions-it/MapStore2/blob/master/web/client/components/import/dragZone/enhancers/processFiles.jsx#L105

adding || type === 'application/geo+json'

additionally there are some tests that are failing, do you have time to have a look at that?

i have tested using these
geosol training.zip
keep in mind that for now geojson needs to be projected in 4326 to work fine in longitudinal profile tool for example

@tdipisa tdipisa modified the milestones: 2024.01.01, 2024.01.02, 2024.02.00 May 28, 2024
@tdipisa tdipisa modified the milestones: 2024.02.00, 2024.02.01 Oct 2, 2024
@tdipisa tdipisa added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Oct 8, 2024
@tdipisa tdipisa modified the milestones: 2024.02.01, 2025.01.00 Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch bug External Contribution Internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix importing geojson files on firefox on linux/openbsd
3 participants