Skip to content

Commit 72f710e

Browse files
ShaileshParmar11Copilot
ShaileshParmar11
and
Copilot
authored
Address feedback for rule library feature (#25573)
* Address feedback for rule library feature - Added "supported-service-plural" and "supported-services-help" keys to various language files (en-us, es-es, fr-fr, gl-es, he-he, ja-jp, ko-kr, mr-in, nl-nl, pr-pr, pt-br, pt-pt, ru-ru, th-th, tr-tr, zh-cn, zh-tw) to enhance user interface localization. - Implemented utility functions in TestDefinitionUtils to determine if a test definition is external based on its platforms, with corresponding unit tests to ensure functionality. * Refactor TestDefinitionForm to improve readability and maintainability by consolidating conditional logic for read-only fields and optimizing imports. * Address code review feedback for rule library feature (#25590) * Initial plan * Address review feedback: fix Select component props, E2E test casing, test cleanup, and add translations Co-authored-by: ShaileshParmar11 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: ShaileshParmar11 <[email protected]> * Enhance TestDefinitionForm and utils: streamline validator logic, improve test accessibility, and refine platform checks --------- Co-authored-by: Copilot <[email protected]>
1 parent 00d6c1d commit 72f710e

File tree

26 files changed

+1650
-471
lines changed

26 files changed

+1650
-471
lines changed

‎openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/DataQuality/RulesLibrary.spec.ts‎

Lines changed: 828 additions & 348 deletions
Large diffs are not rendered by default.

‎openmetadata-ui/src/main/resources/ui/src/components/RulesLibrary/TestDefinitionForm/TestDefinitionForm.component.tsx‎

Lines changed: 120 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -11,30 +11,16 @@
1111
* limitations under the License.
1212
*/
1313

14-
import {
15-
MinusCircleOutlined,
16-
PlusOutlined,
17-
QuestionCircleOutlined,
18-
} from '@ant-design/icons';
19-
import {
20-
Button,
21-
Card,
22-
Drawer,
23-
Form,
24-
Input,
25-
Select,
26-
Space,
27-
Switch,
28-
Tooltip,
29-
Typography,
30-
} from 'antd';
14+
import { MinusCircleOutlined, PlusOutlined } from '@ant-design/icons';
15+
import { Button, Card, Drawer, Form, Input, Select, Space, Switch } from 'antd';
3116
import { AxiosError } from 'axios';
3217
import { compare } from 'fast-json-patch';
3318
import React, { useMemo, useState } from 'react';
3419
import { useTranslation } from 'react-i18next';
3520
import { ReactComponent as CloseIcon } from '../../../assets/svg/close.svg';
3621
import { CSMode } from '../../../enums/codemirror.enum';
3722
import { CreateTestDefinition } from '../../../generated/api/tests/createTestDefinition';
23+
import { DatabaseServiceType } from '../../../generated/entity/services/databaseService';
3824
import {
3925
DataQualityDimensions,
4026
DataType,
@@ -47,7 +33,9 @@ import {
4733
createTestDefinition,
4834
patchTestDefinition,
4935
} from '../../../rest/testAPI';
36+
import { handleSearchFilterOption } from '../../../utils/CommonUtils';
5037
import { createScrollToErrorHandler } from '../../../utils/formUtils';
38+
import { isExternalTestDefinition } from '../../../utils/TestDefinitionUtils';
5139
import { showSuccessToast } from '../../../utils/ToastUtils';
5240
import AlertBar from '../../AlertBar/AlertBar';
5341
import FormItemLabel from '../../common/Form/FormItemLabel';
@@ -71,6 +59,23 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
7159
const isEditMode = Boolean(initialValues);
7260
const scrollToError = useMemo(() => createScrollToErrorHandler(), []);
7361

62+
const isReadOnlyField = useMemo(() => {
63+
if (!initialValues) {
64+
return false;
65+
}
66+
67+
const isExternalTest = isExternalTestDefinition(initialValues);
68+
69+
return isExternalTest && isEditMode;
70+
}, [initialValues, isEditMode]);
71+
72+
const databaseServiceTypes = useMemo(() => {
73+
return Object.values(DatabaseServiceType).map((service) => ({
74+
label: service,
75+
value: service,
76+
}));
77+
}, []);
78+
7479
const handleSubmit = async (values: TestDefinition) => {
7580
setIsSubmitting(true);
7681
setErrorMessage('');
@@ -90,6 +95,14 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
9095
})
9196
);
9297
} else {
98+
let validatorClass: string | undefined;
99+
if (values.sqlExpression) {
100+
validatorClass =
101+
values.entityType === EntityType.Column
102+
? 'ColumnRuleLibrarySqlExpressionValidator'
103+
: 'TableRuleLibrarySqlExpressionValidator';
104+
}
105+
93106
const payload: CreateTestDefinition = {
94107
name: values.name,
95108
displayName: values.displayName,
@@ -99,12 +112,9 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
99112
testPlatforms: values.testPlatforms,
100113
dataQualityDimension: values.dataQualityDimension,
101114
supportedDataTypes: values.supportedDataTypes,
115+
supportedServices: values.supportedServices,
102116
parameterDefinition: values.parameterDefinition,
103-
validatorClass: values.sqlExpression
104-
? values.entityType === EntityType.Column
105-
? 'ColumnRuleLibrarySqlExpressionValidator'
106-
: 'TableRuleLibrarySqlExpressionValidator'
107-
: undefined,
117+
validatorClass,
108118
};
109119
await createTestDefinition(payload);
110120
showSuccessToast(
@@ -184,6 +194,7 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
184194
testPlatforms: initialValues?.testPlatforms || [
185195
TestPlatform.OpenMetadata,
186196
],
197+
supportedServices: initialValues?.supportedServices || [],
187198
}}
188199
layout="vertical"
189200
onFinish={handleSubmit}
@@ -234,25 +245,29 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
234245
/>
235246
</Form.Item>
236247

237-
<Form.Item name="sqlExpression">
238-
<CodeEditor
239-
refreshEditor
240-
showCopyButton
241-
className="custom-query-editor query-editor-h-200"
242-
mode={{ name: CSMode.SQL }}
243-
title={
244-
<div className="ant-form-item-label">
245-
<label className="d-flex align-items-center">
246-
<Typography.Text className="form-label-title">
247-
{t('label.sql-query')}
248-
</Typography.Text>
249-
<Tooltip title={t('message.test-definition-sql-query-help')}>
250-
<QuestionCircleOutlined className="ant-form-item-tooltip" />
251-
</Tooltip>
252-
</label>
253-
</div>
254-
}
255-
/>
248+
<Form.Item
249+
label={
250+
<FormItemLabel
251+
helperText={t('message.test-definition-sql-query-help')}
252+
label={t('label.sql-query')}
253+
/>
254+
}
255+
name="sqlExpression">
256+
{isReadOnlyField ? (
257+
<Input.TextArea
258+
disabled
259+
rows={8}
260+
style={{ fontFamily: 'monospace', fontSize: '12px' }}
261+
value={initialValues?.sqlExpression}
262+
/>
263+
) : (
264+
<CodeEditor
265+
refreshEditor
266+
showCopyButton
267+
className="custom-query-editor query-editor-h-200"
268+
mode={{ name: CSMode.SQL }}
269+
/>
270+
)}
256271
</Form.Item>
257272

258273
<Form.Item
@@ -267,7 +282,7 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
267282
},
268283
]}>
269284
<Select
270-
disabled={isEditMode}
285+
disabled={isReadOnlyField}
271286
id="entityType"
272287
options={Object.values(EntityType).map((type) => ({
273288
label: type,
@@ -291,6 +306,7 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
291306
},
292307
]}>
293308
<Select
309+
disabled={isReadOnlyField}
294310
id="testPlatforms"
295311
mode="multiple"
296312
options={Object.values(TestPlatform).map((platform) => ({
@@ -317,10 +333,29 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
317333
/>
318334
</Form.Item>
319335

336+
<Form.Item
337+
label={
338+
<FormItemLabel
339+
helperText={t('message.supported-services-help')}
340+
label={t('label.supported-service-plural')}
341+
/>
342+
}
343+
name="supportedServices">
344+
<Select
345+
showSearch
346+
disabled={isReadOnlyField}
347+
filterOption={handleSearchFilterOption}
348+
mode="multiple"
349+
options={databaseServiceTypes}
350+
placeholder={t('message.empty-means-all-services')}
351+
/>
352+
</Form.Item>
353+
320354
<Form.Item
321355
label={t('label.supported-data-type-plural')}
322356
name="supportedDataTypes">
323357
<Select
358+
disabled={isReadOnlyField}
324359
mode="multiple"
325360
options={Object.values(DataType).map((dataType) => ({
326361
label: dataType,
@@ -344,7 +379,11 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
344379
<>
345380
{fields.map(({ key, name, ...restField }) => (
346381
<Card
347-
extra={<MinusCircleOutlined onClick={() => remove(name)} />}
382+
extra={
383+
!isReadOnlyField && (
384+
<MinusCircleOutlined onClick={() => remove(name)} />
385+
)
386+
}
348387
key={key}
349388
size="small"
350389
style={{ marginTop: 16 }}
@@ -361,21 +400,28 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
361400
}),
362401
},
363402
]}>
364-
<Input placeholder={t('label.parameter-name')} />
403+
<Input
404+
disabled={isReadOnlyField}
405+
placeholder={t('label.parameter-name')}
406+
/>
365407
</Form.Item>
366408

367409
<Form.Item
368410
{...restField}
369411
label={t('label.display-name')}
370412
name={[name, 'displayName']}>
371-
<Input placeholder={t('label.parameter-display-name')} />
413+
<Input
414+
disabled={isReadOnlyField}
415+
placeholder={t('label.parameter-display-name')}
416+
/>
372417
</Form.Item>
373418

374419
<Form.Item
375420
{...restField}
376421
label={t('label.description')}
377422
name={[name, 'description']}>
378423
<Input.TextArea
424+
disabled={isReadOnlyField}
379425
placeholder={t('label.parameter-description')}
380426
rows={2}
381427
/>
@@ -394,6 +440,9 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
394440
},
395441
]}>
396442
<Select
443+
showSearch
444+
disabled={isReadOnlyField}
445+
filterOption={handleSearchFilterOption}
397446
options={Object.values(TestDataType).map((type) => ({
398447
label: type,
399448
value: type,
@@ -404,35 +453,39 @@ const TestDefinitionForm: React.FC<TestDefinitionFormProps> = ({
404453
/>
405454
</Form.Item>
406455

407-
<Form.Item
408-
{...restField}
409-
label={t('label.required')}
410-
name={[name, 'required']}
411-
valuePropName="checked">
412-
<Switch />
413-
</Form.Item>
456+
<div className="d-flex items-center gap-2">
457+
<label>{t('label.required')}</label>
458+
<Form.Item
459+
noStyle
460+
{...restField}
461+
name={[name, 'required']}
462+
valuePropName="checked">
463+
<Switch disabled={isReadOnlyField} />
464+
</Form.Item>
465+
</div>
414466
</Card>
415467
))}
416-
<Button
417-
block
418-
icon={<PlusOutlined />}
419-
style={{ marginTop: 16 }}
420-
type="dashed"
421-
onClick={() => add()}>
422-
{t('label.add-entity', { entity: t('label.parameter') })}
423-
</Button>
468+
{!isReadOnlyField && (
469+
<Button
470+
block
471+
icon={<PlusOutlined />}
472+
style={{ marginTop: 16 }}
473+
type="dashed"
474+
onClick={() => add()}>
475+
{t('label.add-entity', { entity: t('label.parameter') })}
476+
</Button>
477+
)}
424478
</>
425479
)}
426480
</Form.List>
427481
</Form.Item>
428482
{isEditMode && (
429-
<Form.Item
430-
label={t('label.enabled')}
431-
name="enabled"
432-
style={{ marginTop: 16 }}
433-
valuePropName="checked">
434-
<Switch />
435-
</Form.Item>
483+
<div className="d-flex items-center gap-2" style={{ marginTop: 16 }}>
484+
<label>{t('label.enabled')}</label>
485+
<Form.Item noStyle name="enabled" valuePropName="checked">
486+
<Switch />
487+
</Form.Item>
488+
</div>
436489
)}
437490
</Form>
438491
</Drawer>

0 commit comments

Comments
 (0)