Skip to content

Commit ea2e966

Browse files
alyssajoyneralyssajoyner
authored
Fix overflow and toggle bugs (#1409)
1 parent d87df96 commit ea2e966

8 files changed

+175
-57
lines changed

‎src/views/config-v2/AdditionalSettingsSection.tsx‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
import { ConfigSubSection } from 'components/experimental/ConfigSection';
22
import allLabels from './labelsV2';
33
import React, { ChangeEvent, useState } from 'react';
4-
import { DataSourcePluginOptionsEditorProps, onUpdateDatasourceJsonDataOption } from '@grafana/data';
4+
import {
5+
DataSourcePluginOptionsEditorProps,
6+
onUpdateDatasourceJsonDataOption,
7+
onUpdateDatasourceJsonDataOptionChecked,
8+
} from '@grafana/data';
59
import { AliasTableEntry, CHConfig, CHCustomSetting, CHLogsConfig, CHSecureConfig, CHTracesConfig } from 'types/config';
610
import { AliasTableConfig } from 'components/configEditor/AliasTableConfig';
711
import { DefaultDatabaseTableConfig } from 'components/configEditor/DefaultDatabaseTableConfig';
@@ -119,7 +123,7 @@ export const AdditionalSettingsSection = (props: Props) => {
119123
label={
120124
<>
121125
<Text variant="h3">4. {CONFIG_SECTION_HEADERS[3].label}</Text>
122-
<Badge text="optional" color="blue" className={styles.badge} />
126+
<Badge text="optional" color="darkgrey" className={styles.badge} />
123127
</>
124128
}
125129
isOpen={!!CONFIG_SECTION_HEADERS[3].isOpen}
@@ -166,7 +170,7 @@ export const AdditionalSettingsSection = (props: Props) => {
166170
}}
167171
onValidateSqlChange={(e) => {
168172
trackClickhouseConfigV2QuerySettings({ validateSql: e.currentTarget.checked });
169-
onUpdateDatasourceJsonDataOption(props, 'validateSql')(e);
173+
onUpdateDatasourceJsonDataOptionChecked(props, 'validateSql')(e);
170174
}}
171175
/>
172176
<Divider />
@@ -221,7 +225,7 @@ export const AdditionalSettingsSection = (props: Props) => {
221225
data-testid={labels.enableRowLimit.testid}
222226
onChange={(e) => {
223227
trackClickhouseConfigV2EnableRowLimitToggle({ rowLimitEnabled: e.currentTarget.checked });
224-
onUpdateDatasourceJsonDataOption(props, 'enableRowLimit')(e);
228+
onUpdateDatasourceJsonDataOptionChecked(props, 'enableRowLimit')(e);
225229
}}
226230
/>
227231
</Field>

‎src/views/config-v2/CHConfigEditor.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ const getStyles = (theme: GrafanaTheme2) => ({
7171
top: '100px',
7272
alignSelf: 'flex-start',
7373
maxHeight: 'calc(100vh - 100px)',
74-
overflowY: 'auto',
74+
overflow: 'hidden',
7575
}),
7676
requiredFields: css({
7777
marginBottom: theme.spacing(2),

‎src/views/config-v2/HttpHeadersConfigV2.test.tsx‎

Lines changed: 139 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,38 @@ import React from 'react';
22
import { render, screen, fireEvent } from '@testing-library/react';
33
import { HttpHeadersConfigV2 } from './HttpHeadersConfigV2';
44
import { selectors } from 'selectors';
5+
import { Protocol } from 'types/config';
6+
import { createTestProps } from './helpers';
57

68
describe('HttpHeadersConfigV2', () => {
79
const onHttpHeadersChange = jest.fn();
8-
const onForwardGrafanaHeadersChange = jest.fn();
10+
const onOptionsChangeMock = jest.fn();
911
let consoleSpy: jest.SpyInstance;
1012

13+
const defaultProps = createTestProps({
14+
options: {
15+
jsonData: {
16+
host: '',
17+
secure: false,
18+
protocol: Protocol.Native,
19+
port: undefined,
20+
pdcInjected: false,
21+
},
22+
secureJsonData: {},
23+
secureJsonFields: {},
24+
},
25+
mocks: {
26+
onOptionsChange: onOptionsChangeMock,
27+
},
28+
});
29+
1130
const renderWith = (overrides?: Partial<React.ComponentProps<typeof HttpHeadersConfigV2>>) => {
1231
const props: React.ComponentProps<typeof HttpHeadersConfigV2> = {
32+
...defaultProps,
1333
headers: [],
1434
forwardGrafanaHeaders: false,
1535
secureFields: {},
1636
onHttpHeadersChange,
17-
onForwardGrafanaHeadersChange,
1837
...(overrides || {}),
1938
};
2039
return render(<HttpHeadersConfigV2 {...props} />);
@@ -89,6 +108,123 @@ describe('HttpHeadersConfigV2', () => {
89108
const forwardCb = screen.getByLabelText(/forward grafana http headers to data source/i) as HTMLInputElement;
90109
fireEvent.click(forwardCb);
91110

92-
expect(onForwardGrafanaHeadersChange).toHaveBeenCalledWith(true);
111+
expect(onOptionsChangeMock).toHaveBeenCalled();
112+
expect(onOptionsChangeMock).toHaveBeenLastCalledWith(
113+
expect.objectContaining({
114+
jsonData: expect.objectContaining({ forwardGrafanaHeaders: true }),
115+
})
116+
);
117+
});
118+
119+
describe('HttpHeadersConfigV2', () => {
120+
const onHttpHeadersChange = jest.fn();
121+
const onOptionsChangeMock = jest.fn();
122+
let consoleSpy: jest.SpyInstance;
123+
124+
const defaultProps = createTestProps({
125+
options: {
126+
jsonData: {
127+
host: '',
128+
secure: false,
129+
protocol: Protocol.Native,
130+
port: undefined,
131+
pdcInjected: false,
132+
},
133+
secureJsonData: {},
134+
secureJsonFields: {},
135+
},
136+
mocks: {
137+
onOptionsChange: onOptionsChangeMock,
138+
},
139+
});
140+
141+
const renderWith = (overrides?: Partial<React.ComponentProps<typeof HttpHeadersConfigV2>>) => {
142+
const props: React.ComponentProps<typeof HttpHeadersConfigV2> = {
143+
...defaultProps,
144+
headers: [],
145+
forwardGrafanaHeaders: false,
146+
secureFields: {},
147+
onHttpHeadersChange,
148+
...(overrides || {}),
149+
};
150+
return render(<HttpHeadersConfigV2 {...props} />);
151+
};
152+
153+
beforeEach(() => {
154+
// Mock console.error to suppress React act() warnings
155+
consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
156+
jest.clearAllMocks();
157+
});
158+
159+
afterEach(() => {
160+
consoleSpy.mockRestore();
161+
});
162+
163+
it('renders top label, Add header button, and forward checkbox', () => {
164+
renderWith();
165+
166+
expect(screen.getByText(/custom http headers/i)).toBeInTheDocument();
167+
expect(screen.getByRole('button', { name: /add header/i })).toBeInTheDocument();
168+
expect(screen.getByLabelText(/forward grafana http headers to data source/i)).toBeInTheDocument();
169+
});
170+
171+
it('adds a new header editor when Add header is clicked', () => {
172+
renderWith();
173+
174+
const before = screen.queryAllByTestId(selectors.components.Config.HttpHeaderConfig.headerEditor).length;
175+
fireEvent.click(screen.getByTestId(selectors.components.Config.HttpHeaderConfig.addHeaderButton));
176+
const after = screen.getAllByTestId(selectors.components.Config.HttpHeaderConfig.headerEditor).length;
177+
178+
expect(after).toBe(before + 1);
179+
expect(onHttpHeadersChange).not.toHaveBeenCalled();
180+
});
181+
182+
it('renders any initial headers passed in', () => {
183+
renderWith({
184+
headers: [
185+
{ name: 'X-Auth', value: 'abc', secure: false },
186+
{ name: 'Foo', value: 'bar', secure: true },
187+
],
188+
});
189+
190+
const editors = screen.getAllByTestId(selectors.components.Config.HttpHeaderConfig.headerEditor);
191+
expect(editors.length).toBe(2);
192+
expect(screen.getAllByTestId(selectors.components.Config.HttpHeaderConfig.headerNameInput)[0]).toHaveValue(
193+
'X-Auth'
194+
);
195+
expect(screen.getAllByTestId(selectors.components.Config.HttpHeaderConfig.headerNameInput)[1]).toHaveValue('Foo');
196+
});
197+
198+
it('removes a header and calls onHttpHeadersChange when Remove is clicked', () => {
199+
renderWith({
200+
headers: [
201+
{ name: 'A', value: '1', secure: false },
202+
{ name: 'B', value: '2', secure: false },
203+
],
204+
});
205+
206+
const before = screen.getAllByTestId(selectors.components.Config.HttpHeaderConfig.headerEditor).length;
207+
const removeButtons = screen.getAllByTestId('trash-alt');
208+
fireEvent.click(removeButtons[0]);
209+
210+
expect(onHttpHeadersChange).toHaveBeenCalled();
211+
const next = onHttpHeadersChange.mock.lastCall?.[0];
212+
expect(next.length).toBe(before - 1);
213+
expect(next.find((h: any) => h.name === 'A')).toBeUndefined();
214+
});
215+
216+
it('toggles "Forward Grafana headers" and updated forwardGrafanaHeaders value to true', () => {
217+
renderWith({ forwardGrafanaHeaders: false });
218+
219+
const forwardCb = screen.getByLabelText(/forward grafana http headers to data source/i) as HTMLInputElement;
220+
fireEvent.click(forwardCb);
221+
222+
expect(onOptionsChangeMock).toHaveBeenCalled();
223+
expect(onOptionsChangeMock).toHaveBeenLastCalledWith(
224+
expect.objectContaining({
225+
jsonData: expect.objectContaining({ forwardGrafanaHeaders: true }),
226+
})
227+
);
228+
});
93229
});
94230
});

‎src/views/config-v2/HttpHeadersConfigV2.tsx‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,16 @@
11
import React, { ChangeEvent, useMemo, useState } from 'react';
22
import { Input, Field, SecretInput, Button, Stack, Checkbox, Box } from '@grafana/ui';
3-
import { CHHttpHeader } from 'types/config';
3+
import { CHConfig, CHHttpHeader, CHSecureConfig } from 'types/config';
44
import allLabels from './labelsV2';
55
import { styles } from 'styles';
66
import { selectors as allSelectors } from 'selectors';
7-
import { KeyValue } from '@grafana/data';
7+
import { DataSourcePluginOptionsEditorProps, KeyValue, onUpdateDatasourceJsonDataOptionChecked } from '@grafana/data';
88

9-
interface HttpHeadersConfigProps {
9+
interface HttpHeadersConfigProps extends DataSourcePluginOptionsEditorProps<CHConfig, CHSecureConfig> {
1010
headers?: CHHttpHeader[];
1111
forwardGrafanaHeaders?: boolean;
1212
secureFields: KeyValue<boolean>;
1313
onHttpHeadersChange: (v: CHHttpHeader[]) => void;
14-
onForwardGrafanaHeadersChange: (v: boolean) => void;
1514
}
1615

1716
export const HttpHeadersConfigV2 = (props: HttpHeadersConfigProps) => {
@@ -40,9 +39,9 @@ export const HttpHeadersConfigV2 = (props: HttpHeadersConfigProps) => {
4039
onHttpHeadersChange(nextHeaders);
4140
};
4241

43-
const updateForwardGrafanaHeaders = (value: boolean) => {
44-
setForwardGrafanaHeaders(value);
45-
props.onForwardGrafanaHeadersChange(value);
42+
const updateForwardGrafanaHeaders = (e: React.SyntheticEvent<HTMLInputElement, Event>) => {
43+
setForwardGrafanaHeaders(e.currentTarget.checked);
44+
onUpdateDatasourceJsonDataOptionChecked(props, 'forwardGrafanaHeaders')(e);
4645
};
4746

4847
return (
@@ -73,11 +72,10 @@ export const HttpHeadersConfigV2 = (props: HttpHeadersConfigProps) => {
7372
</>
7473
</Field>
7574

76-
{/* Use 'checked' instead of 'value' */}
7775
<Checkbox
7876
label={labels.forwardGrafanaHeaders.label}
7977
checked={forwardGrafanaHeaders}
80-
onChange={(e) => updateForwardGrafanaHeaders(e.currentTarget.checked)}
78+
onChange={(e) => updateForwardGrafanaHeaders(e)}
8179
/>
8280
</div>
8381
);
@@ -122,7 +120,6 @@ const HttpHeaderEditorV2 = (props: HttpHeaderEditorProps) => {
122120
/>
123121
</Field>
124122

125-
{/* Avoid 'grow'; use flex prop that Box supports */}
126123
<Box flex="1 1 auto">
127124
<Field label={headerValueLabel} aria-label={headerValueLabel}>
128125
{secure ? (
@@ -148,7 +145,6 @@ const HttpHeaderEditorV2 = (props: HttpHeaderEditorProps) => {
148145
</Box>
149146

150147
{!isSecureConfigured && (
151-
// Use 'checked' instead of 'value'
152148
<Checkbox
153149
label={labels.secureLabel}
154150
checked={secure}

‎src/views/config-v2/HttpProtocolSettingsSection.test.tsx‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ describe('HttpProtocolSettingsSection', () => {
4343
...defaultProps.options,
4444
jsonData: { ...defaultProps.options.jsonData, protocol: Protocol.Native },
4545
}}
46-
onSwitchToggle={jest.fn()}
4746
/>
4847
);
4948

@@ -52,7 +51,7 @@ describe('HttpProtocolSettingsSection', () => {
5251
});
5352

5453
it('calls onOptionsChange when HTTP path is changed', () => {
55-
render(<HttpProtocolSettingsSection {...defaultProps} onSwitchToggle={jest.fn()} />);
54+
render(<HttpProtocolSettingsSection {...defaultProps} />);
5655

5756
const pathInput = screen.getByLabelText(/path/i);
5857
fireEvent.change(pathInput, { target: { value: '/api' } });
@@ -61,7 +60,7 @@ describe('HttpProtocolSettingsSection', () => {
6160
});
6261

6362
it('toggles Optional HTTP settings open/closed via the button (icon changes)', () => {
64-
render(<HttpProtocolSettingsSection {...defaultProps} onSwitchToggle={jest.fn()} />);
63+
render(<HttpProtocolSettingsSection {...defaultProps} />);
6564

6665
expect(screen.getByTestId('angle-right')).toBeInTheDocument();
6766

‎src/views/config-v2/HttpProtocolSettingsSection.tsx‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,11 @@ import { css } from '@emotion/css';
77
import { onHttpHeadersChange } from 'views/CHConfigEditorHooks';
88
import { HttpHeadersConfigV2 } from './HttpHeadersConfigV2';
99

10-
export interface HttpProtocolSettingsSectionProps extends DataSourcePluginOptionsEditorProps<CHConfig, CHSecureConfig> {
11-
onSwitchToggle: (
12-
key: keyof Pick<
13-
CHConfig,
14-
'secure' | 'validateSql' | 'enableSecureSocksProxy' | 'forwardGrafanaHeaders' | 'enableRowLimit'
15-
>,
16-
value: boolean
17-
) => void;
18-
}
10+
export interface HttpProtocolSettingsSectionProps
11+
extends DataSourcePluginOptionsEditorProps<CHConfig, CHSecureConfig> {}
1912

2013
export const HttpProtocolSettingsSection = (props: HttpProtocolSettingsSectionProps) => {
21-
const { options, onOptionsChange, onSwitchToggle } = props;
14+
const { options, onOptionsChange } = props;
2215
const { jsonData, secureJsonFields } = options;
2316
const labels = allLabels.components.Config.ConfigEditor;
2417

@@ -54,11 +47,12 @@ export const HttpProtocolSettingsSection = (props: HttpProtocolSettingsSectionPr
5447
</Button>
5548
{optionalHttpIsOpen && (
5649
<HttpHeadersConfigV2
50+
options={options}
51+
onOptionsChange={onOptionsChange}
5752
headers={jsonData.httpHeaders}
5853
forwardGrafanaHeaders={jsonData.forwardGrafanaHeaders}
5954
secureFields={secureJsonFields}
6055
onHttpHeadersChange={(headers) => onHttpHeadersChange(headers, options, onOptionsChange)}
61-
onForwardGrafanaHeadersChange={(forward) => onSwitchToggle('forwardGrafanaHeaders', forward)}
6256
/>
6357
)}
6458
</div>

‎src/views/config-v2/ServerAndEncryptionSection.tsx‎

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
import React from 'react';
2-
import { DataSourcePluginOptionsEditorProps, GrafanaTheme2, onUpdateDatasourceJsonDataOption } from '@grafana/data';
2+
import {
3+
DataSourcePluginOptionsEditorProps,
4+
GrafanaTheme2,
5+
onUpdateDatasourceJsonDataOption,
6+
onUpdateDatasourceJsonDataOptionChecked,
7+
} from '@grafana/data';
38
import {
49
Box,
510
CollapsableSection,
@@ -61,22 +66,6 @@ export const ServerAndEncryptionSection = (props: Props) => {
6166
});
6267
};
6368

64-
const onSwitchToggle = (
65-
key: keyof Pick<
66-
CHConfig,
67-
'secure' | 'validateSql' | 'enableSecureSocksProxy' | 'forwardGrafanaHeaders' | 'enableRowLimit'
68-
>,
69-
value: boolean
70-
) => {
71-
onOptionsChange({
72-
...options,
73-
jsonData: {
74-
...options.jsonData,
75-
[key]: value,
76-
},
77-
});
78-
};
79-
8069
return (
8170
<Box
8271
borderStyle="solid"
@@ -184,11 +173,11 @@ export const ServerAndEncryptionSection = (props: Props) => {
184173
checked={jsonData.secure || false}
185174
onChange={(e) => {
186175
trackClickhouseConfigV2SecureConnectionChecked({ secureConnection: e.currentTarget.checked });
187-
onSwitchToggle('secure', e.currentTarget.checked);
176+
onUpdateDatasourceJsonDataOptionChecked(props, 'secure')(e);
188177
}}
189178
/>
190179
</div>
191-
<HttpProtocolSettingsSection onSwitchToggle={onSwitchToggle} {...props} />
180+
<HttpProtocolSettingsSection {...props} />
192181
</CollapsableSection>
193182
</Box>
194183
);

0 commit comments

Comments
 (0)