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

Enabling users to upload custom GeoJSON #1437

Closed

Conversation

Shivamdhar
Copy link
Contributor

@Shivamdhar Shivamdhar commented Apr 8, 2022

Description

  • Enables users to upload custom GeoJSON files
  • Adds UI elements, API layer and backend connectivity code with OpenSearch cluster and geospatial plugin

Issues Resolved

#1408
Task - Allow users to upload custom vector map

Check List

  • New functionality includes testing.

         yarn test:jest src/plugins/region_map/*                                  
         yarn run v1.22.18
         $ node scripts/jest src/plugins/region_map/common src/plugins/region_map/config.ts 
         src/plugins/region_map/opensearch_dashboards.json src/plugins/region_map/package.json 
         src/plugins/region_map/public src/plugins/region_map/server src/plugins/region_map/target
         PASS  src/plugins/region_map/public/region_map_fn.test.js
         PASS  src/plugins/region_map/public/components/empty_prompt.test.tsx
         PASS  src/plugins/region_map/public/components/vector_upload_options.test.tsx
         PASS  src/plugins/region_map/public/components/custom_vector_upload.test.tsx
        
        Test Suites: 4 passed, 4 total
        Tests:       16 passed, 16 total
        Snapshots:   5 passed, 5 total
        Time:        10.499 s
    
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
         Test Suites: 50 passed, 50 total
         Tests:       5 skipped, 438 passed, 443 total
         Snapshots:   77 passed, 77 total
         Time:        329.189 s
         Ran all test suites.
                  │ info [opensearch] cleanup complete
         Done in 332.29s.
        
      • yarn test:ftr
  • New functionality has been documented.
    RFC can be accessed here: [RFC] - Upload Custom GeoJSON as vector map geospatial#47

  • Commits are signed per the DCO using --signoff

Todo:

  • Add unit tests

@Shivamdhar Shivamdhar changed the title Custom geojson feature [Custom-GeoJSON] Enabling users to upload custom GeoJSON Apr 8, 2022
@Shivamdhar Shivamdhar changed the title [Custom-GeoJSON] Enabling users to upload custom GeoJSON Enabling users to upload custom GeoJSON Apr 8, 2022
@Shivamdhar Shivamdhar marked this pull request as ready for review April 12, 2022 18:11
@Shivamdhar Shivamdhar requested a review from a team as a code owner April 12, 2022 18:11
@VijayanB
Copy link
Member

@kavilla From our POV this PR looks good other than some minor feedback. Can you please take a look at it? Thanks.

Signed-off-by: Shivam Dhar <[email protected]>
@Shivamdhar Shivamdhar requested a review from junqiu-lei April 25, 2022 23:37
@tmarkley
Copy link
Contributor

@VijayanB @Shivamdhar the core Dashboards team has expressed some concerns about this approach in the original issue: #1408. Can we discuss the open questions first before proceeding?

@Shivamdhar
Copy link
Contributor Author

@VijayanB @Shivamdhar the core Dashboards team has expressed some concerns about this approach in the original issue: #1408. Can we discuss the open questions first before proceeding?

Sure Tommy, will jot down the thoughts in that thread by eod. Thanks for taking out time : )

@tmarkley
Copy link
Contributor

Excluding the open conversation about the approach itself, we need test coverage before this is merged. There are no new tests here. If you need help determining how to implement tests, please let us know.

@Shivamdhar
Copy link
Contributor Author

Shivamdhar commented Apr 29, 2022

Excluding the open conversation about the approach itself, we need test coverage before this is merged. There are no new tests here. If you need help determining how to implement tests, please let us know.

Yes, I'm on it. I'll be adding tests to the PR as I'm getting to learn the different testing strategies on this side as well. Currently, working on adding snapshot tests. Will keep the PR updated with the test cases as and when I write them.

Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

Blocking this until tests are added. Take a look at our guide for Writing Tests and feel free to reach out if you need additional help!

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2022

Codecov Report

Merging #1437 (dec62ce) into main (0afba22) will increase coverage by 0.03%.
The diff coverage is 80.57%.

@@            Coverage Diff             @@
##             main    #1437      +/-   ##
==========================================
+ Coverage   68.09%   68.12%   +0.03%     
==========================================
  Files        3072     3078       +6     
  Lines       59033    59175     +142     
  Branches     8928     8950      +22     
==========================================
+ Hits        40199    40314     +115     
- Misses      16647    16668      +21     
- Partials     2187     2193       +6     
Impacted Files Coverage Δ
src/plugins/region_map/public/services.ts 20.00% <20.00%> (ø)
...on_map/public/components/vector_upload_options.tsx 85.00% <85.00%> (ø)
src/plugins/region_map/common/constants/shared.ts 100.00% <100.00%> (ø)
...ion_map/public/components/custom_vector_upload.tsx 100.00% <100.00%> (ø)
...gins/region_map/public/components/empty_prompt.tsx 100.00% <100.00%> (ø)
src/plugins/region_map/public/region_map_fn.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0afba22...dec62ce. Read the comment docs.

Signed-off-by: Shivam Dhar <[email protected]>
Signed-off-by: Shivam Dhar <[email protected]>
Signed-off-by: Shivam Dhar <[email protected]>
Signed-off-by: Shivam Dhar <[email protected]>
@Shivamdhar
Copy link
Contributor Author

We have decided to move the component (Import custom map tab) out from region map and make it part of the dashboards-maps plugin. Closing this PR in favor of opensearch-project/dashboards-maps#7.

Thank you all for you suggestions and guidance.

@Shivamdhar Shivamdhar closed this May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants