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

GeoJSON, allow uploading and convert JSON<->GeoJson and vice versa #5

Closed
odscjames opened this issue Oct 19, 2022 · 18 comments
Closed
Assignees

Comments

@odscjames
Copy link
Collaborator

Depends on Open-Telecoms-Data/lib-cove-ofds#25

@odscjames
Copy link
Collaborator Author

The UI is ready for testing, tho the GeoJson->Json conversion still needs work - this is being worked on elsewhere: ttps://github.com/Open-Telecoms-Data/ofdskit/issues/1

@duncandewhurst
Copy link
Contributor

duncandewhurst commented Nov 6, 2022

I get a Server Error (500) when I submit the following files:

Regarding the UI, can we use a tabbed interface with a tab for each upload format and with collapsible boxes for each upload method (like in the OCDS Data Review Tool)? That way, it's easier for users to switch between the different formats and upload methods.

I sketched an example of what I'm thinking:

image

@odscjames
Copy link
Collaborator Author

Thought: A tabbed interface doesn't leave any room to describe what JSON/ GeoJSON/ etc is, and it would be easy for the user to miss the tabs, jump straight to the forms then fill out the form for the wrong format. Having buttons on home page where you have to select the format first solves both those.

But happy to do whatever, I'll try to talk to someone to pick a solution.

@duncandewhurst
Copy link
Contributor

A tabbed interface doesn't leave any room to describe what JSON/ GeoJSON/ etc is

Can you explain what you think needs to be described? We can add an introductory paragraph before the tabbed interface.

@odscjames
Copy link
Collaborator Author

Less technical people won't be clear on what the different formats mean. Things like "files ending .csv" may help. Especially as there will be a 4th tab - CSV's. (Difference between CSV's and spreadsheets that has to be dealt with somewhere - CSV's is many files and spreadsheets is only one) And many people don't read introductory paragraphs.

@duncandewhurst
Copy link
Contributor

If people have managed to generate an OFDS file in one of the supported publication formats, they should be able to select that publication format in an interface.

Spreadsheets are not a supported publication format so we can omit those for now.

And many people don't read introductory paragraphs.

I don't know what this is based on. The paragraph can simply be:

Use the form below to submit your data. You can submit data in either JSON, GeoJSON or CSV format. For more information, see the publication format reference.

@odscjames
Copy link
Collaborator Author

I get a Server Error (500) when I submit the following files:

Ah - it was only allowing files with ".json" endings which is what I was testing with. Files with ".geojson" endings now allowed, and fixed it so the user sees a proper error message to.

@duncandewhurst
Copy link
Contributor

I've done some testing with the nodes.geojson and spans.geojson files from 0.1-dev branch (results). Looks good in general, but a few things to do:

  • Upload interface: add paste and URL options.
  • GeoJSON > JSON conversion:
    • the phases array is missing from the JSON file, which results in failures against the phase reference additional check
    • the organisations array is missing from the JSON file, which results in failures against the organisation references additional check
  • There is a structure and format error and many additional fields errors, but I think that is because the data is being checked against the alpha schema

@duncandewhurst
Copy link
Contributor

The priority outstanding action for this issue is:

  • GeoJSON > JSON conversion:
    • the phases array is missing from the JSON file, which results in failures against the phase reference additional check
    • the organisations array is missing from the JSON file, which results in failures against the organisation references additional check

@odscjames
Copy link
Collaborator Author

odscjames commented Nov 9, 2022

This may have just been testing on an old branch (but I'm not sure how that happened). EDIT: this is now on live

Look in https://github.com/Open-Telecoms-Data/lib-cove-ofds/tree/main/tests/fixtures/geojson_to_json - the "phases_1*" and the "organisations_1_*" files, are the "expected" files what you want to see?

@duncandewhurst
Copy link
Contributor

duncandewhurst commented Nov 9, 2022

This may have just been testing on an old branch (but I'm not sure how that happened). EDIT: this is now on live

Ah, maybe that was it. However, testing on live, I noticed that jsontogeojson does not dereference phases.funders and geojsontojson does not add the organisations in phases.funders to organisations. I made a PR to fix the issue in jsontogeojson - please can you fix the issue in geojsontojson?

Look in https://github.com/Open-Telecoms-Data/lib-cove-ofds/tree/main/tests/fixtures/geojson_to_json - the "phases_1*" and the "organisations_1_*" files, are the "expected" files what you want to see?

In phases_1.expected.json, .name is missing from all the phase references.

In organisations_1.expected.json, .name is missing from all the organisation references. The test data should also include phases.funders to test the (missing) functionality mentioned above.

@odscjames
Copy link
Collaborator Author

Spun 2 conversion issues to issues in libcoveofds, where they will need to be fixed.

Is there are more on the UI to do here?

@duncandewhurst
Copy link
Contributor

Thanks! Just one issue to fix in the UI, which I only noticed today: we should allow users to upload either only or nodes file or only a spans file, currently the UI requires them to upload both.

@odscjames
Copy link
Collaborator Author

Just one issue to fix in the UI, which I only noticed today: we should allow users to upload either only or nodes file or only a spans file, currently the UI requires them to upload both.

Now on live, to test

@lgs85
Copy link

lgs85 commented Nov 30, 2022

This is working well. One optional thing we may want to consider: when we have a bunch of node info in spans.geojson, and no nodes.geojson file we could in theory populate the nodes array when converting to JSON. I can't see that many use cases for this though so not sure it's worth the effort. If others agree we can close this I think.

@duncandewhurst
Copy link
Contributor

when we have a bunch of node info in spans.geojson

By node info, do you mean features in which geometry.type is 'Point' or do you mean the dereferenced node information in Span.start and Span.end?

If it's the former, I don't think we want to support that as it doesn't conform to the GeoJSON publication format. However, we should probably implement a warning to the user.

If it's the latter, then that sounds good to me.

@odscjames have we implemented a warning if a user submits nodes and spans files with inconsistent nodes data?

Side note: I'm wondering whether revisiting the design choice to have separate files for spans and nodes might reduce the complexity of the conversion tooling.

@lgs85
Copy link

lgs85 commented Dec 1, 2022

By node info, do you mean features in which geometry.type is 'Point' or do you mean the dereferenced node information in Span.start and Span.end?

The latter. @odscjames and I discussed and agreed to move this to a separate issue so we can prioritise accordingly. Closing this issue as completed.

@lgs85 lgs85 closed this as completed Dec 1, 2022
@odscjames
Copy link
Collaborator Author

have we implemented a warning if a user submits nodes and spans files with inconsistent nodes data?

Logged in Open-Telecoms-Data/lib-cove-ofds#52

Side note: I'm wondering whether revisiting the design choice to have separate files for spans and nodes might reduce the complexity of the conversion tooling.

We should have a chat around this. Also noting Open-Telecoms-Data/lib-cove-ofds#21 - discussion of this and the other suggestions in this ticket between @lgs85 and me got into many topics in this area.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants