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

[BUG] show region blocked warning config not respected #2042

Merged

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Aug 2, 2022

Description

The configuration wasn't passed down to the options that was
expecting it to be available for the plugin to run the conditional
logic.

Value was undefined for region map. Also, cleaned up the vega map
view code.

Signed-off-by: Kawika Avilla kavilla414@gmail.com

Issues Resolved

#2041

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

The configuration wasn't passed down to the options that was
expecting it to be available for the plugin to run the conditional
logic.

Value was undefined for region map. Also, cleaned up the vega map
view code.

Issue:

opensearch-project#2041

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
@kavilla kavilla requested a review from a team as a code owner August 2, 2022 08:57
@kavilla kavilla added bug Something isn't working maps Issues or PRs related to the Maps Service backport 2.x labels Aug 2, 2022
@kavilla kavilla linked an issue Aug 2, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

Codecov Report

Merging #2042 (b2021a7) into main (54c2cdc) will increase coverage by 0.00%.
The diff coverage is 25.00%.

@@           Coverage Diff           @@
##             main    #2042   +/-   ##
=======================================
  Coverage   67.49%   67.50%           
=======================================
  Files        3076     3077    +1     
  Lines       59144    59188   +44     
  Branches     8989     9003   +14     
=======================================
+ Hits        39919    39954   +35     
- Misses      17041    17048    +7     
- Partials     2184     2186    +2     
Impacted Files Coverage Δ
.../maps_legacy/public/map/base_maps_visualization.js 1.94% <0.00%> (-0.02%) ⬇️
...plugins/maps_legacy/public/map/service_settings.js 1.60% <0.00%> (-0.03%) ⬇️
...ns/vis_type_vega/public/vega_view/vega_map_view.js 54.34% <ø> (ø)
src/plugins/vis_type_vega/public/services.ts 100.00% <100.00%> (ø)
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 83.67% <0.00%> (-4.09%) ⬇️
packages/osd-optimizer/src/node/cache.ts 50.00% <0.00%> (-2.78%) ⬇️
...ore/public/chrome/ui/header/header_breadcrumbs.tsx 100.00% <0.00%> (ø)
...s/vis_type_vega/public/vega_view/vega_base_view.js 55.55% <0.00%> (ø)
packages/osd-std/src/validate_object.ts 91.30% <0.00%> (ø)
...c/plugins/vis_type_vega/public/data_model/utils.ts 73.33% <0.00%> (+34.87%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

A screenshot would be helpful to illustrate what this actually changes or fixes.

@kavilla
Copy link
Member Author

kavilla commented Aug 2, 2022

A screenshot would be helpful to illustrate what this actually changes or fixes.

To recreate go to region map and you can see that settings.options does not include showRegionBlockedWarning as the code is expecting.

Example on main after setting the config to true:
Screen Shot 2022-08-02 at 4 42 56 PM

But on this branch you can see the value for example:
Screen Shot 2022-08-02 at 4 46 04 PM

Right now the config value does get set but the logic checks an object that never included the config value so this adds it to the object that the logic is expecting to read. The reason why this made it through is because right now maps_legacy jest tests are ignored. So I created a followup issue to re-enable them and why they were disabled in the first place here: #2057

@kavilla kavilla merged commit 5fb4143 into opensearch-project:main Aug 2, 2022
@kavilla kavilla deleted the avillk/fix_region_block_message branch August 2, 2022 23:48
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 2, 2022
The configuration wasn't passed down to the options that was
expecting it to be available for the plugin to run the conditional
logic.

Value was undefined for region map. Also, cleaned up the vega map
view code.

Issue:

#2041

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
(cherry picked from commit 5fb4143)
kavilla added a commit that referenced this pull request Aug 3, 2022
The configuration wasn't passed down to the options that was
expecting it to be available for the plugin to run the conditional
logic.

Value was undefined for region map. Also, cleaned up the vega map
view code.

Issue:

#2041

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
(cherry picked from commit 5fb4143)

Co-authored-by: Kawika Avilla <kavilla414@gmail.com>
CPTNB pushed a commit to CPTNB/OpenSearch-Dashboards that referenced this pull request Aug 8, 2022
…oject#2042)

The configuration wasn't passed down to the options that was
expecting it to be available for the plugin to run the conditional
logic.

Value was undefined for region map. Also, cleaned up the vega map
view code.

Issue:

opensearch-project#2041

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x bug Something isn't working maps Issues or PRs related to the Maps Service v2.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Maps showRegionBlockedConfig doesn't work
5 participants