From ce210eebdeeb374611e5b273379a889244f64288 Mon Sep 17 00:00:00 2001 From: Ross Mabbett <92495987+rtexelm@users.noreply.github.com> Date: Tue, 26 Mar 2024 12:54:03 -0400 Subject: [PATCH] fix(Chart Annotation modal): Table and Superset annotation options will paginate, exceeding previous max limit 100 (#27022) --- .../AnnotationLayer.jsx | 350 ++++++++++++------ .../AnnotationLayer.test.tsx | 125 +++++-- 2 files changed, 329 insertions(+), 146 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx index 563b94682..e29befc80 100644 --- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx +++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx @@ -33,12 +33,12 @@ import { withTheme, } from '@superset-ui/core'; import SelectControl from 'src/explore/components/controls/SelectControl'; +import { AsyncSelect } from 'src/components'; import TextControl from 'src/explore/components/controls/TextControl'; import CheckboxControl from 'src/explore/components/controls/CheckboxControl'; import PopoverSection from 'src/components/PopoverSection'; import ControlHeader from 'src/explore/components/ControlHeader'; import { EmptyStateSmall } from 'src/components/EmptyState'; -import { FILTER_OPTIONS_LIMIT } from 'src/explore/constants'; import { ANNOTATION_SOURCE_TYPES, ANNOTATION_TYPES, @@ -194,28 +194,46 @@ class AnnotationLayer extends React.PureComponent { hideLine, // refData isNew: !name, - isLoadingOptions: true, - valueOptions: [], + slice: null, }; this.submitAnnotation = this.submitAnnotation.bind(this); this.deleteAnnotation = this.deleteAnnotation.bind(this); this.applyAnnotation = this.applyAnnotation.bind(this); - this.fetchOptions = this.fetchOptions.bind(this); + this.isValidForm = this.isValidForm.bind(this); + // Handlers this.handleAnnotationType = this.handleAnnotationType.bind(this); this.handleAnnotationSourceType = this.handleAnnotationSourceType.bind(this); - this.handleValue = this.handleValue.bind(this); - this.isValidForm = this.isValidForm.bind(this); + this.handleSelectValue = this.handleSelectValue.bind(this); + this.handleTextValue = this.handleTextValue.bind(this); + // Fetch related functions + this.fetchOptions = this.fetchOptions.bind(this); + this.fetchCharts = this.fetchCharts.bind(this); + this.fetchNativeAnnotations = this.fetchNativeAnnotations.bind(this); + this.fetchAppliedAnnotation = this.fetchAppliedAnnotation.bind(this); + this.fetchSliceData = this.fetchSliceData.bind(this); + this.shouldFetchSliceData = this.shouldFetchSliceData.bind(this); + this.fetchAppliedChart = this.fetchAppliedChart.bind(this); + this.fetchAppliedNativeAnnotation = + this.fetchAppliedNativeAnnotation.bind(this); + this.shouldFetchAppliedAnnotation = + this.shouldFetchAppliedAnnotation.bind(this); } componentDidMount() { - const { annotationType, sourceType, isLoadingOptions } = this.state; - this.fetchOptions(annotationType, sourceType, isLoadingOptions); + if (this.shouldFetchAppliedAnnotation()) { + const { value } = this.state; + /* The value prop is the id of the chart/native. This function will set + value in state to an object with the id as value.value to be used by + AsyncSelect */ + this.fetchAppliedAnnotation(value); + } } componentDidUpdate(prevProps, prevState) { - if (prevState.sourceType !== this.state.sourceType) { - this.fetchOptions(this.state.annotationType, this.state.sourceType, true); + if (this.shouldFetchSliceData(prevState)) { + const { value } = this.state; + this.fetchSliceData(value.value); } } @@ -237,6 +255,20 @@ class AnnotationLayer extends React.PureComponent { return sources; } + shouldFetchAppliedAnnotation() { + const { value, sourceType } = this.state; + return value && requiresQuery(sourceType); + } + + shouldFetchSliceData(prevState) { + const { value, sourceType } = this.state; + const isChart = + sourceType !== ANNOTATION_SOURCE_TYPES.NATIVE && + requiresQuery(sourceType); + const valueIsNew = value && prevState.value !== value; + return valueIsNew && isChart; + } + isValidFormulaAnnotation(expression, annotationType) { if (annotationType === ANNOTATION_TYPES.FORMULA) { return isValidExpression(expression); @@ -276,6 +308,7 @@ class AnnotationLayer extends React.PureComponent { annotationType, sourceType: null, value: null, + slice: null, }); } @@ -283,13 +316,17 @@ class AnnotationLayer extends React.PureComponent { const { sourceType: prevSourceType } = this.state; if (prevSourceType !== sourceType) { - this.setState({ sourceType, value: null, isLoadingOptions: true }); + this.setState({ + sourceType, + value: null, + slice: null, + }); } } - handleValue(value) { + handleSelectValue(selectedValueObject) { this.setState({ - value, + value: selectedValueObject, descriptionColumns: [], intervalEndColumn: null, timeColumn: null, @@ -298,74 +335,172 @@ class AnnotationLayer extends React.PureComponent { }); } - fetchOptions(annotationType, sourceType, isLoadingOptions) { - if (isLoadingOptions) { - if (sourceType === ANNOTATION_SOURCE_TYPES.NATIVE) { - const queryParams = rison.encode({ - page: 0, - page_size: FILTER_OPTIONS_LIMIT, - }); - SupersetClient.get({ - endpoint: `/api/v1/annotation_layer/?q=${queryParams}`, - }).then(({ json }) => { - const layers = json - ? json.result.map(layer => ({ - value: layer.id, - label: layer.name, - })) - : []; - this.setState({ - isLoadingOptions: false, - valueOptions: layers, - }); - }); - } else if (requiresQuery(sourceType)) { - const queryParams = rison.encode({ - filters: [ - { - col: 'id', - opr: 'chart_owned_created_favored_by_me', - value: true, - }, - ], - order_column: 'slice_name', - order_direction: 'asc', - page: 0, - page_size: FILTER_OPTIONS_LIMIT, - }); - SupersetClient.get({ - endpoint: `/api/v1/chart/?q=${queryParams}`, - }).then(({ json }) => { - const registry = getChartMetadataRegistry(); - this.setState({ - isLoadingOptions: false, - valueOptions: json.result - .filter(x => { - const metadata = registry.get(x.viz_type); - return metadata && metadata.canBeAnnotationType(annotationType); - }) - .map(x => ({ - value: x.id, - label: x.slice_name, - slice: { - ...x, - data: { - ...x.form_data, - groupby: x.form_data.groupby?.map(column => - getColumnLabel(column), - ), - }, - }, - })), - }); - }); - } else { + handleTextValue(inputValue) { + this.setState({ + value: inputValue, + }); + } + + fetchNativeAnnotations = async (search, page, pageSize) => { + const queryParams = rison.encode({ + filters: [ + { + col: 'name', + opr: 'ct', + value: search, + }, + ], + columns: ['id', 'name'], + page, + page_size: pageSize, + }); + + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/annotation_layer/?q=${queryParams}`, + }); + + const { result, count } = json; + + const layersArray = result.map(layer => ({ + value: layer.id, + label: layer.name, + })); + + return { + data: layersArray, + totalCount: count, + }; + }; + + fetchCharts = async (search, page, pageSize) => { + const { annotationType } = this.state; + + const queryParams = rison.encode({ + filters: [ + { col: 'slice_name', opr: 'chart_all_text', value: search }, + { + col: 'id', + opr: 'chart_owned_created_favored_by_me', + value: true, + }, + ], + columns: ['id', 'slice_name', 'viz_type'], + order_column: 'slice_name', + order_direction: 'asc', + page, + page_size: pageSize, + }); + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/chart/?q=${queryParams}`, + }); + + const { result, count } = json; + const registry = getChartMetadataRegistry(); + + const chartsArray = result + .filter(chart => { + const metadata = registry.get(chart.viz_type); + return metadata && metadata.canBeAnnotationType(annotationType); + }) + .map(chart => ({ + value: chart.id, + label: chart.slice_name, + viz_type: chart.viz_type, + })); + + return { + data: chartsArray, + totalCount: count, + }; + }; + + fetchOptions = (search, page, pageSize) => { + const { sourceType } = this.state; + + if (sourceType === ANNOTATION_SOURCE_TYPES.NATIVE) { + return this.fetchNativeAnnotations(search, page, pageSize); + } + return this.fetchCharts(search, page, pageSize); + }; + + fetchSliceData = id => { + const queryParams = rison.encode({ + columns: ['query_context'], + }); + SupersetClient.get({ + endpoint: `/api/v1/chart/${id}?q=${queryParams}`, + }).then(({ json }) => { + const { result } = json; + const queryContext = result.query_context; + const formData = JSON.parse(queryContext).form_data; + const dataObject = { + data: { + ...formData, + groupby: formData.groupby?.map(column => getColumnLabel(column)), + }, + }; + this.setState({ + slice: dataObject, + }); + }); + }; + + fetchAppliedChart(id) { + const { annotationType } = this.state; + const registry = getChartMetadataRegistry(); + const queryParams = rison.encode({ + columns: ['slice_name', 'query_context', 'viz_type'], + }); + SupersetClient.get({ + endpoint: `/api/v1/chart/${id}?q=${queryParams}`, + }).then(({ json }) => { + const { result } = json; + const sliceName = result.slice_name; + const queryContext = result.query_context; + const vizType = result.viz_type; + const formData = JSON.parse(queryContext).form_data; + const metadata = registry.get(vizType); + const canBeAnnotationType = + metadata && metadata.canBeAnnotationType(annotationType); + if (canBeAnnotationType) { this.setState({ - isLoadingOptions: false, - valueOptions: [], + value: { + value: id, + label: sliceName, + }, + slice: { + data: { + ...formData, + groupby: formData.groupby?.map(column => getColumnLabel(column)), + }, + }, }); } + }); + } + + fetchAppliedNativeAnnotation(id) { + SupersetClient.get({ + endpoint: `/api/v1/annotation_layer/${id}`, + }).then(({ json }) => { + const { result } = json; + const layer = result; + this.setState({ + value: { + value: layer.id, + label: layer.name, + }, + }); + }); + } + + fetchAppliedAnnotation(id) { + const { sourceType } = this.state; + + if (sourceType === ANNOTATION_SOURCE_TYPES.NATIVE) { + return this.fetchAppliedNativeAnnotation(id); } + return this.fetchAppliedChart(id); } deleteAnnotation() { @@ -374,6 +509,7 @@ class AnnotationLayer extends React.PureComponent { } applyAnnotation() { + const { value, sourceType } = this.state; if (this.isValidForm()) { const annotationFields = [ 'name', @@ -385,7 +521,6 @@ class AnnotationLayer extends React.PureComponent { 'width', 'showMarkers', 'hideLine', - 'value', 'overrides', 'show', 'showLabel', @@ -401,6 +536,10 @@ class AnnotationLayer extends React.PureComponent { } }); + // Prepare newAnnotation.value for use in runAnnotationQuery() + const applicableValue = requiresQuery(sourceType) ? value.value : value; + newAnnotation.value = applicableValue; + if (newAnnotation.color === AUTOMATIC_COLOR) { newAnnotation.color = null; } @@ -415,29 +554,19 @@ class AnnotationLayer extends React.PureComponent { this.props.close(); } - renderOption(option) { + renderChartHeader(label, description, value) { return ( - - {option.label} - + ); } renderValueConfiguration() { - const { - annotationType, - sourceType, - value, - valueOptions, - isLoadingOptions, - } = this.state; + const { annotationType, sourceType, value } = this.state; let label = ''; let description = ''; if (requiresQuery(sourceType)) { @@ -462,20 +591,15 @@ class AnnotationLayer extends React.PureComponent { } if (requiresQuery(sourceType)) { return ( - } /> ); @@ -490,7 +614,7 @@ class AnnotationLayer extends React.PureComponent { label={label} placeholder="" value={value} - onChange={this.handleValue} + onChange={this.handleTextValue} validationErrors={ !this.isValidFormulaAnnotation(value, annotationType) ? [t('Bad formula.')] @@ -507,14 +631,18 @@ class AnnotationLayer extends React.PureComponent { annotationType, sourceType, value, - valueOptions, + slice, overrides, titleColumn, timeColumn, intervalEndColumn, descriptionColumns, } = this.state; - const { slice } = valueOptions.find(x => x.value === value) || {}; + + if (!slice || !value) { + return ''; + } + if (sourceType !== ANNOTATION_SOURCE_TYPES.NATIVE && slice) { const columns = (slice.data.groupby || []) .concat(slice.data.all_columns || []) diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx index 914be7361..813a8ebcb 100644 --- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx +++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx @@ -31,21 +31,37 @@ const defaultProps = { annotationType: ANNOTATION_TYPES_METADATA.FORMULA.value, }; +const nativeLayerApiRoute = 'glob:*/api/v1/annotation_layer/*'; +const chartApiRoute = /\/api\/v1\/chart\/\?q=.+/; +const chartApiWithIdRoute = /\/api\/v1\/chart\/\w+\?q=.+/; + +const withIdResult = { + result: { + slice_name: 'Mocked Slice', + query_context: JSON.stringify({ + form_data: { + groupby: ['country'], + }, + }), + viz_type: 'line', + }, +}; + beforeAll(() => { const supportedAnnotationTypes = Object.values(ANNOTATION_TYPES_METADATA).map( value => value.value, ); - fetchMock.get('glob:*/api/v1/annotation_layer/*', { - result: [{ label: 'Chart A', value: 'a' }], + fetchMock.get(nativeLayerApiRoute, { + result: [{ name: 'Chart A', id: 'a' }], }); - fetchMock.get('glob:*/api/v1/chart/*', { - result: [ - { id: 'a', slice_name: 'Chart A', viz_type: 'table', form_data: {} }, - ], + fetchMock.get(chartApiRoute, { + result: [{ id: 'a', slice_name: 'Chart A', viz_type: 'table' }], }); + fetchMock.get(chartApiWithIdRoute, withIdResult); + setupColors(); getChartMetadataRegistry().registerValue( @@ -144,7 +160,7 @@ test('triggers removeAnnotationLayer and close when remove button is clicked', a expect(close).toHaveBeenCalled(); }); -test('renders chart options', async () => { +test('fetches Superset annotation layer options', async () => { await waitForRender({ annotationType: ANNOTATION_TYPES_METADATA.EVENT.value, }); @@ -153,12 +169,37 @@ test('renders chart options', async () => { ); userEvent.click(screen.getByText('Superset annotation')); expect(await screen.findByText('Annotation layer')).toBeInTheDocument(); + userEvent.click( + screen.getByRole('combobox', { name: 'Annotation layer value' }), + ); + expect(await screen.findByText('Chart A')).toBeInTheDocument(); + expect(fetchMock.calls(nativeLayerApiRoute).length).toBe(1); +}); +test('fetches chart options', async () => { + await waitForRender({ + annotationType: ANNOTATION_TYPES_METADATA.EVENT.value, + }); userEvent.click( screen.getByRole('combobox', { name: 'Annotation source type' }), ); userEvent.click(screen.getByText('Table')); expect(await screen.findByText('Chart')).toBeInTheDocument(); + userEvent.click( + screen.getByRole('combobox', { name: 'Annotation layer value' }), + ); + expect(await screen.findByText('Chart A')).toBeInTheDocument(); + expect(fetchMock.calls(chartApiRoute).length).toBe(1); +}); + +test('fetches chart on mount if value present', async () => { + await waitForRender({ + name: 'Test', + value: 'a', + annotationType: ANNOTATION_TYPES_METADATA.EVENT.value, + sourceType: 'Table', + }); + expect(fetchMock.calls(chartApiWithIdRoute).length).toBe(1); }); test('keeps apply disabled when missing required fields', async () => { @@ -171,7 +212,7 @@ test('keeps apply disabled when missing required fields', async () => { ); expect(await screen.findByText('Chart A')).toBeInTheDocument(); userEvent.click(screen.getByText('Chart A')); - + await screen.findByText(/title column/i); userEvent.click(screen.getByRole('button', { name: 'Automatic Color' })); userEvent.click( screen.getByRole('combobox', { name: 'Annotation layer title column' }), @@ -197,47 +238,61 @@ test('keeps apply disabled when missing required fields', async () => { expect(screen.getByRole('button', { name: 'Apply' })).toBeDisabled(); }); -test.skip('Disable apply button if formula is incorrect', async () => { - // TODO: fix flaky test that passes locally but fails on CI +test('Disable apply button if formula is incorrect', async () => { await waitForRender({ name: 'test' }); - userEvent.clear(screen.getByLabelText('Formula')); - userEvent.type(screen.getByLabelText('Formula'), 'x+1'); + const formulaInput = screen.getByRole('textbox', { name: 'Formula' }); + const applyButton = screen.getByRole('button', { name: 'Apply' }); + const okButton = screen.getByRole('button', { name: 'OK' }); + + userEvent.type(formulaInput, 'x+1'); + expect(formulaInput).toHaveValue('x+1'); await waitFor(() => { - expect(screen.getByLabelText('Formula')).toHaveValue('x+1'); - expect(screen.getByRole('button', { name: 'OK' })).toBeEnabled(); - expect(screen.getByRole('button', { name: 'Apply' })).toBeEnabled(); + expect(okButton).toBeEnabled(); + expect(applyButton).toBeEnabled(); }); - userEvent.clear(screen.getByLabelText('Formula')); - userEvent.type(screen.getByLabelText('Formula'), 'y = x*2+1'); + userEvent.clear(formulaInput); await waitFor(() => { - expect(screen.getByLabelText('Formula')).toHaveValue('y = x*2+1'); - expect(screen.getByRole('button', { name: 'OK' })).toBeEnabled(); - expect(screen.getByRole('button', { name: 'Apply' })).toBeEnabled(); + expect(formulaInput).toHaveValue(''); + }); + userEvent.type(formulaInput, 'y = x*2+1'); + expect(formulaInput).toHaveValue('y = x*2+1'); + await waitFor(() => { + expect(okButton).toBeEnabled(); + expect(applyButton).toBeEnabled(); }); - userEvent.clear(screen.getByLabelText('Formula')); - userEvent.type(screen.getByLabelText('Formula'), 'y+1'); + userEvent.clear(formulaInput); await waitFor(() => { - expect(screen.getByLabelText('Formula')).toHaveValue('y+1'); - expect(screen.getByRole('button', { name: 'OK' })).toBeDisabled(); - expect(screen.getByRole('button', { name: 'Apply' })).toBeDisabled(); + expect(formulaInput).toHaveValue(''); + }); + userEvent.type(formulaInput, 'y+1'); + expect(formulaInput).toHaveValue('y+1'); + await waitFor(() => { + expect(okButton).toBeDisabled(); + expect(applyButton).toBeDisabled(); }); - userEvent.clear(screen.getByLabelText('Formula')); - userEvent.type(screen.getByLabelText('Formula'), 'x+'); + userEvent.clear(formulaInput); await waitFor(() => { - expect(screen.getByLabelText('Formula')).toHaveValue('x+'); - expect(screen.getByRole('button', { name: 'OK' })).toBeDisabled(); - expect(screen.getByRole('button', { name: 'Apply' })).toBeDisabled(); + expect(formulaInput).toHaveValue(''); + }); + userEvent.type(formulaInput, 'x+'); + expect(formulaInput).toHaveValue('x+'); + await waitFor(() => { + expect(okButton).toBeDisabled(); + expect(applyButton).toBeDisabled(); }); - userEvent.clear(screen.getByLabelText('Formula')); - userEvent.type(screen.getByLabelText('Formula'), 'y = z+1'); + userEvent.clear(formulaInput); await waitFor(() => { - expect(screen.getByLabelText('Formula')).toHaveValue('y = z+1'); - expect(screen.getByRole('button', { name: 'OK' })).toBeDisabled(); - expect(screen.getByRole('button', { name: 'Apply' })).toBeDisabled(); + expect(formulaInput).toHaveValue(''); + }); + userEvent.type(formulaInput, 'y = z+1'); + expect(formulaInput).toHaveValue('y = z+1'); + await waitFor(() => { + expect(okButton).toBeDisabled(); + expect(applyButton).toBeDisabled(); }); });