[Maps] Do not check count for blended layers when layer is not visible

[nreese]

fixes #66460

Steps to view bug:

1. install sample web logs and create a Documents layer with clustering enabled.
2. hide layer
3. Change time range
4. Notice how _search request is made for the count of documents. Hidden layers should not make any requests.

This PR fixes the issue by consolidating the layer visibility check in map action syncDataForLayer that calls syncData

[elasticmachine]

Pinging @elastic/kibana-gis (Team:Geo)

[thomasneirynck]

thanks, good catch.

Would you mind adding a test for this? imho syncData is core enough for a unit test.

[nreese]

The PR moves the layer visibility check into map_actions where syncData is called. This removes all the duplicated logic of checking visibility in each syncData implementation.

I am removing 7.8.0 label since this PR is now more complicated then the simple fix.

[nreese]

Removed EMS base map layer from layerList. Tests using these maps look at mapbox style. EMS base map layer bloats mapbox style making the inspector very slow to render. Removing that layer speeds up tests a lot.

[thomasneirynck]

thx, good call. Didn't occur to me, but when we moved to vector-tiles, this added a large amount of layers to the mb-map

[nreese]

@elasticmachine merge upstream

[thomasneirynck]

Can we export syncData and add a test to map_selectors.test.js

Since we're patching, I'd strongly recommend adding a test that verifies this change.

e.g. export syncDataForLayer from map_actions and add something like:

describe('syncDataForLayer', () => {
    let isVisible;
    let showAtZoomLevel;
    let startLoading;
    const mockLayer = {
      isVisible() {
        return isVisible;
      },
      getId() {
        return 'foobar';
      },
      showAtZoomLevel() {
        return showAtZoomLevel;
      },
      syncData() {
        startLoading = true;
      },
    };
    beforeEach(() => {
      startLoading = false;
    });

    it('should not sync when visible layerproperty is false', async () => {
      isVisible = false;
      showAtZoomLevel = true;
      const action = syncDataForLayer(mockLayer);
      await action(dispatchMock, getStoreMock);
      expect(startLoading).toEqual(false);
    });

    it('should not sync when outside zoom range', async () => {
      isVisible = true;
      showAtZoomLevel = false;
      const action = syncDataForLayer(mockLayer);
      await action(dispatchMock, getStoreMock);
      expect(startLoading).toEqual(false);
    });

    it('should only sync when both visible and in zoom range', async () => {
      isVisible = true;
      showAtZoomLevel = true;
      const action = syncDataForLayer(mockLayer);
      await action(dispatchMock, getStoreMock);
      expect(startLoading).toEqual(true);
    });
  });

[thomasneirynck]

thanks for pulling this out here. it really helps when the business-logic is gathered in the actions rather than sprinkled across multiple classes.

[thomasneirynck]

thx, good call. Didn't occur to me, but when we moved to vector-tiles, this added a large amount of layers to the mb-map

[nreese]

@elasticmachine merge upstream

[nreese]

Can we export syncData and add a test to map_actions.test.js

Those tests will not work when converted to TS. I would like to move this into another PR that starts to break map_actions into smaller files that can be converted to typescript. Then we can add tests to those smaller files. I will get a PR out for this next.

[nreese]

created #66821 to track unit tests one data request actions have been pulled out of map_actions

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 3f11520

History

• 💚 Build #48459 succeeded 3f11520
• 💚 Build #48161 succeeded 98bac55
• 💔 Build #48084 failed 7ac07f7
• 💔 Build #47981 failed 382a3ce
• 💔 Build #47977 failed af8ff7d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream