From d0c19db3d12008a3b502811b6c7e15f0df9dcafa Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Tue, 2 Apr 2024 11:30:19 -0700 Subject: [PATCH] Fix zoom level type error in custom layer (#605) (#606) * Fix zoom level type error in custom layer Signed-off-by: Junqiu Lei (cherry picked from commit 4d21bf0809ba5d7b2802fcb7162dc169f2664530) Co-authored-by: Junqiu Lei --- CHANGELOG.md | 1 + .../layer_basic_settings.test.tsx | 113 +++++++++++++ .../layer_config/layer_basic_settings.tsx | 14 +- public/model/customLayerFunctions.test.ts | 154 ++++++++++++++++++ public/model/customLayerFunctions.ts | 11 +- 5 files changed, 288 insertions(+), 5 deletions(-) create mode 100644 public/components/layer_config/layer_basic_settings.test.tsx create mode 100644 public/model/customLayerFunctions.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0186244d..287942cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Features ### Enhancements ### Bug Fixes +* Fix zoom level type error in custom layer ([#605](https://github.com/opensearch-project/dashboards-maps/pull/605)) ### Infrastructure ### Documentation ### Maintenance diff --git a/public/components/layer_config/layer_basic_settings.test.tsx b/public/components/layer_config/layer_basic_settings.test.tsx new file mode 100644 index 00000000..05f18170 --- /dev/null +++ b/public/components/layer_config/layer_basic_settings.test.tsx @@ -0,0 +1,113 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import React from 'react'; +import { LayerBasicSettings } from './layer_basic_settings'; +import { MapLayerSpecification } from '../../model/mapLayerType'; +import { DASHBOARDS_MAPS_LAYER_TYPE, MAX_LAYER_NAME_LIMIT } from '../../../common'; +import { shallow } from 'enzyme'; +import { EuiDualRange, EuiFieldText, EuiFormRow, EuiRange, EuiTextArea } from '@elastic/eui'; + +describe('LayerBasicSettings', () => { + let wrapper: any; + const mockLayerConfig: MapLayerSpecification = { + name: 'Test Layer', + id: 'test-layer', + type: DASHBOARDS_MAPS_LAYER_TYPE.OPENSEARCH_MAP, + description: 'Some description', + zoomRange: [1, 20], + visibility: 'visible', + opacity: 80, + }; + + const mockProps = { + selectedLayerConfig: mockLayerConfig, + setSelectedLayerConfig: jest.fn(), + setIsUpdateDisabled: jest.fn(), + isLayerExists: jest.fn(), + }; + + beforeEach(() => { + wrapper = shallow(); + }); + + it('should render with correct props', () => { + expect(wrapper.find(EuiFieldText).prop('value')).toBe(mockLayerConfig.name); + expect(wrapper.find(EuiTextArea).prop('value')).toBe(mockLayerConfig.description); + expect(wrapper.find(EuiDualRange).prop('value')).toEqual(mockLayerConfig.zoomRange); + expect(wrapper.find(EuiRange).prop('value')).toEqual(mockLayerConfig.opacity); + }); + + it('should call setSelectedLayerConfig with updated zoom range on zoom level change', () => { + const newZoomRange = [2, 15]; + wrapper.find(EuiDualRange).simulate('change', newZoomRange); + + expect(mockProps.setSelectedLayerConfig).toHaveBeenCalledWith({ + ...mockLayerConfig, + zoomRange: newZoomRange, + }); + }); + + it('should call setSelectedLayerConfig with updated opacity on opacity change', () => { + const newOpacity = 50; + wrapper.find(EuiRange).simulate('change', { target: { value: newOpacity.toString() } }); + + expect(mockProps.setSelectedLayerConfig).toHaveBeenCalledWith({ + ...mockLayerConfig, + opacity: newOpacity, + }); + }); + + it('should call setSelectedLayerConfig with updated name on name change', () => { + const newName = 'Updated Test Layer'; + wrapper.find(EuiFieldText).simulate('change', { target: { value: newName } }); + + expect(mockProps.setSelectedLayerConfig).toHaveBeenCalledWith({ + ...mockLayerConfig, + name: newName, + }); + }); + + it('should call setSelectedLayerConfig with updated description on description change', () => { + const newDescription = 'Updated description'; + wrapper.find(EuiTextArea).simulate('change', { target: { value: newDescription } }); + + expect(mockProps.setSelectedLayerConfig).toHaveBeenCalledWith({ + ...mockLayerConfig, + description: newDescription, + }); + }); + + it('should display an error for an empty name', () => { + mockProps.isLayerExists.mockReturnValue(false); // Ensure this returns false for this test + const emptyName = ''; + wrapper.find(EuiFieldText).simulate('change', { target: { value: emptyName } }); + + wrapper.update(); + + expect(wrapper.find(EuiFormRow).first().prop('error')).toContain('Name cannot be empty'); + }); + + it('should display an error for a name exceeding the maximum length', () => { + const longName = 'a'.repeat(MAX_LAYER_NAME_LIMIT + 1); // Create a name longer than MAX_LAYER_NAME_LIMIT + wrapper.find(EuiFieldText).simulate('change', { target: { value: longName } }); + + wrapper.update(); + + expect(wrapper.find(EuiFormRow).first().prop('error')).toContain( + `Name should be less than ${MAX_LAYER_NAME_LIMIT} characters` + ); + }); + + it('should display an error for a name that already exists', () => { + mockProps.isLayerExists.mockReturnValue(true); // Simulate that the name already exists + const existingName = 'Existing Layer Name'; + wrapper.find(EuiFieldText).simulate('change', { target: { value: existingName } }); + + wrapper.update(); + + expect(wrapper.find(EuiFormRow).first().prop('error')).toContain('Name already exists'); + }); +}); diff --git a/public/components/layer_config/layer_basic_settings.tsx b/public/components/layer_config/layer_basic_settings.tsx index 2dbbffc6..1ea5b5c3 100644 --- a/public/components/layer_config/layer_basic_settings.tsx +++ b/public/components/layer_config/layer_basic_settings.tsx @@ -25,6 +25,7 @@ import { MAP_LAYER_DEFAULT_OPACITY_STEP, MAX_LAYER_NAME_LIMIT, } from '../../../common'; +import { ValueMember } from '../../../../../src/plugins/opensearch_dashboards_react/public/validated_range/validated_dual_range'; interface Props { selectedLayerConfig: MapLayerSpecification; @@ -41,6 +42,7 @@ export const LayerBasicSettings = ({ }: Props) => { const [invalid, setInvalid] = useState(selectedLayerConfig.name.length === 0); const [errors, setErrors] = useState([]); + const [zoomLevel, setZoomLevel] = useState<[ValueMember, ValueMember]>(selectedLayerConfig.zoomRange as [ValueMember, ValueMember]); const validateName = (name: string) => { if (name?.length === 0) { @@ -73,8 +75,14 @@ export const LayerBasicSettings = ({ const newLayerConfig = { ...selectedLayerConfig, [key]: value }; setSelectedLayerConfig(newLayerConfig); }; - const onZoomChange = (value: number[]) => { - commonUpdate('zoomRange', value); + + const onZoomChange = (value: [ValueMember, ValueMember]) => { + // if value is not empty string, then update the zoom level to layer config + if (value[0] !== '' && value[1] !== '') { + // change value to number type + commonUpdate('zoomRange', value.map((zoomLevel) => Number(zoomLevel))); + } + setZoomLevel(value); }; const onOpacityChange = (e: any) => { @@ -121,7 +129,7 @@ export const LayerBasicSettings = ({ ({ + Map: jest.fn(() => ({ + on: jest.fn(), + off: jest.fn(), + getStyle: jest.fn(() => mockStyle), + setPaintProperty: jest.fn(), + setLayerZoomRange: jest.fn(), + addSource: jest.fn(), + addLayer: jest.fn(), + // @ts-ignore + getSource: jest.fn().mockImplementation((id) => mockStyle.sources[id]), + getLayer: jest + .fn() + .mockImplementation((id) => mockStyle.layers.find((layer) => layer.id === id)), + triggerRepaint: jest.fn(), + _controls: [], + style: { + sourceCaches: {}, + }, + })), +})); + +describe('CustomLayerFunctions', () => { + let map: MapLibre; + let maplibreRef: MaplibreRef; + + beforeEach(() => { + map = new MapLibre({ + container: document.createElement('div'), + style: 'mock-style-url', + }); + + // Initialize sourceCaches with a mock function for each layer that might be accessed + // @ts-ignore + map.style.sourceCaches['existing-layer'] = { + clearTiles: jest.fn(), + update: jest.fn(), + }; + + maplibreRef = { + current: map, + }; + }); + + it('should add a new layer if it does not exist', () => { + const newLayerConfig: CustomLayerSpecification = { + id: 'new-layer', + source: { + // @ts-ignore + type: DASHBOARDS_CUSTOM_MAPS_LAYER_TYPE.TMS, + tiles: ['https://newtiles.example.com/{z}/{x}/{y}.png'], + attribution: 'Test Attribution', + }, + opacity: 80, + zoomRange: [0, 22], + visibility: 'visible', + }; + + CustomLayerFunctions.render(maplibreRef, newLayerConfig); + + expect(map.addSource).toHaveBeenCalledWith(newLayerConfig.id, expect.any(Object)); + expect(map.addLayer).toHaveBeenCalledWith(expect.any(Object)); + }); + + it('should update an existing layer', () => { + const updatedLayerConfig: CustomLayerSpecification = { + id: 'existing-layer', + source: { + // @ts-ignore + type: DASHBOARDS_CUSTOM_MAPS_LAYER_TYPE.TMS, + tiles: ['https://updatedtiles.example.com/{z}/{x}/{y}.png'], + attribution: 'Updated Test Attribution', + }, + opacity: 50, + zoomRange: [0, 15], + visibility: 'visible', + }; + + CustomLayerFunctions.render(maplibreRef, updatedLayerConfig); + + expect(map.setPaintProperty).toHaveBeenCalledWith( + updatedLayerConfig.id, + 'raster-opacity', + updatedLayerConfig.opacity / 100 + ); + expect(map.setLayerZoomRange).toHaveBeenCalledWith( + updatedLayerConfig.id, + updatedLayerConfig.zoomRange[0], + updatedLayerConfig.zoomRange[1] + ); + }); + + it('should convert zoomRange to a numeric array', () => { + const layerConfig = { + id: 'test-layer', + // Assuming zoomRange might be provided as strings from old versions + zoomRange: ['0', '10'], + }; + + // @ts-ignore + const result = applyZoomRangeToLayer(layerConfig); + + // Expected result should be a numeric array + const expectedResult = [0, 10]; + + // Verify the result matches the expected numeric array + expect(result).toEqual(expectedResult); + }); + + it('should handle mixed types in zoomRange', () => { + // Define layerConfig with zoomRange as a mix of numbers and strings + const layerConfig = { + id: 'mixed-type-layer', + zoomRange: [1, '15'], + }; + + // @ts-ignore + const result = applyZoomRangeToLayer(layerConfig); + + const expectedResult = [1, 15]; + + expect(result).toEqual(expectedResult); + }); +}); diff --git a/public/model/customLayerFunctions.ts b/public/model/customLayerFunctions.ts index 51fc2a2d..3685dc3a 100644 --- a/public/model/customLayerFunctions.ts +++ b/public/model/customLayerFunctions.ts @@ -51,6 +51,8 @@ const addNewLayer = (layerConfig: CustomLayerSpecification, maplibreRef: Maplibr tileSize: 256, attribution: layerSource?.attribution, }); + // Convert zoomRange to number[] to avoid type error for backport versions + const zoomRange: number[] = applyZoomRangeToLayer(layerConfig); maplibreInstance.addLayer({ id: layerConfig.id, type: 'raster', @@ -61,8 +63,8 @@ const addNewLayer = (layerConfig: CustomLayerSpecification, maplibreRef: Maplibr layout: { visibility: layerConfig.visibility === 'visible' ? 'visible' : 'none', }, - minzoom: layerConfig.zoomRange[0], - maxzoom: layerConfig.zoomRange[1], + minzoom: zoomRange[0], + maxzoom: zoomRange[1], }); } }; @@ -79,6 +81,11 @@ const getCustomMapURL = (layerConfig: CustomLayerSpecification) => { } }; +export const applyZoomRangeToLayer = (layerConfig: CustomLayerSpecification) => { + // Convert zoomRange to number[] to avoid type error for backport versions + return layerConfig.zoomRange.map((zoom) => Number(zoom)); +}; + export const CustomLayerFunctions = { render: (maplibreRef: MaplibreRef, layerConfig: CustomLayerSpecification) => { return hasLayer(maplibreRef.current!, layerConfig.id)