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

Adding filter of summarized field grouped by latitude and longitude on a map causes table visualization error #30058

Closed
likeshumidity opened this issue Apr 12, 2023 · 3 comments · Fixed by #31952
Assignees
Labels
Priority:P2 Average run of the mill bug .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects Visualization/Maps
Milestone

Comments

@likeshumidity
Copy link
Contributor

Describe the bug

  • Filtering on aggregate column after binning on a map breaks the map and causes unexpected UI Table visualization

Logs
image

To Reproduce
Steps to reproduce the behavior (if you can reproduce the bug using the Sample Database, we will find the issue faster):

  1. Use People from Sample Dataset to create a map with count of rows summary grouped by latitude and longitude (binned)

image

  1. Add a filter on Count > 2

image

  1. Map disappears, Source Table name disappears (top left), and clicking gear to edit visualization shows 3 columns to add that already exist.

image

Expected behavior

  • Expect More Columns in Table view not to show the 3 columns that have already been added.

Information about your Metabase Installation:

You can get this information by going to Admin -> Troubleshooting, or simply post the JSON you see in that page.

  • Metabase version: 45.3.1, 46.1, and master (as of 2023-04-12)

Severity

  • customer reported issue
@kamilmielnik
Copy link
Contributor

kamilmielnik commented Jun 16, 2023

@kamilmielnik
Copy link
Contributor

I looks like the problem is that points.length is not equal to gridSquares.length and therefore the for loop can go out of bounds of either points or gridSquares arrays in LeafletGridHeatMap.

The loops accounts for the index being out of bounds everywhere except the first line. The fix is rather simple:

diff --git a/frontend/src/metabase/visualizations/components/LeafletGridHeatMap.jsx b/frontend/src/metabase/visualizations/components/LeafletGridHeatMap.jsx
index e2d57be15b..ccbc0eb679 100644
--- a/frontend/src/metabase/visualizations/components/LeafletGridHeatMap.jsx
+++ b/frontend/src/metabase/visualizations/components/LeafletGridHeatMap.jsx
@@ -66,8 +66,6 @@ export default class LeafletGridHeatMap extends LeafletMap {
       const longitureValues = points.map(row => row[longitudeIndex]);
 
       for (let i = 0; i < totalSquares; i++) {
-        const [latitude, longiture, metric] = points[i];
-
         if (i >= points.length) {
           gridLayer.removeLayer(gridSquares[i]);
         }
@@ -78,6 +76,8 @@ export default class LeafletGridHeatMap extends LeafletMap {
         }
 
         if (i < points.length) {
+          const [latitude, longiture, metric] = points[i];
+
           gridSquares[i].setStyle({ color: colorScale(metric) });
 
           const [latMin, latMax] = getValueRange(

kamilmielnik added a commit that referenced this issue Jun 28, 2023
kamilmielnik added a commit that referenced this issue Jun 28, 2023
kamilmielnik added a commit that referenced this issue Jun 29, 2023
@kamilmielnik kamilmielnik added the .Reproduced Issues reproduced in test (usually Cypress) label Jun 30, 2023
kamilmielnik added a commit that referenced this issue Jun 30, 2023
…tude on a map causes table visualization error (#31952)

* Fix #30058

* Add missing setting key

* Add a unit test attempt at #30058

* Mock store instead of using deprecated MetabaseSettings.set

* Remove LeafletGridHeatMap unit tests

* Add e2e test for #30058

* Fix typing

* Don't allow undefined "map-tile-server-url"

* Tag the test with repo and issue id
github-actions bot pushed a commit that referenced this issue Jun 30, 2023
…tude on a map causes table visualization error (#31952)

* Fix #30058

* Add missing setting key

* Add a unit test attempt at #30058

* Mock store instead of using deprecated MetabaseSettings.set

* Remove LeafletGridHeatMap unit tests

* Add e2e test for #30058

* Fix typing

* Don't allow undefined "map-tile-server-url"

* Tag the test with repo and issue id
@kamilmielnik kamilmielnik added this to the 0.47 milestone Jun 30, 2023
metabase-bot bot added a commit that referenced this issue Jun 30, 2023
…tude on a map causes table visualization error (#31952) (#32033)

* Fix #30058

* Add missing setting key

* Add a unit test attempt at #30058

* Mock store instead of using deprecated MetabaseSettings.set

* Remove LeafletGridHeatMap unit tests

* Add e2e test for #30058

* Fix typing

* Don't allow undefined "map-tile-server-url"

* Tag the test with repo and issue id

Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
This was referenced Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:P2 Average run of the mill bug .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects Visualization/Maps
Projects
None yet
5 participants