fantonangeli commented on code in PR #3367:
URL:
https://github.com/apache/incubator-kie-tools/pull/3367#discussion_r2588798614
##########
packages/runtime-tools-components/src/utils/ModelConversionTool.ts:
##########
@@ -96,6 +119,13 @@ function convertModel(model: any, schema: Record<string,
any>, ctx: ConversionCo
return;
}
+ if (value === undefined || value === null) {
+ if (propSchema.type === "boolean") {
+ obj[propertyName] = !!value;
Review Comment:
because of the:
```
if (value === undefined || value === null) {...
```
value will never be `true`. What did you mean to write?
##########
packages/runtime-tools-components/src/components/FormRenderer/FormRenderer.tsx:
##########
@@ -58,7 +58,10 @@ export const FormRenderer =
React.forwardRef<FormRendererApi, IOwnProps & OUIAPr
});
// Converting Dates that are in string format into JS Dates so they can be
correctly bound to the uniforms DateField
- const formData = ModelConversionTool.convertStringToDate(model,
formSchema);
+ const formData = ModelConversionTool.normalizeBooleans(
Review Comment:
3 nested calls? Let's avoid too much complexity
##########
packages/uniforms-patternfly/tests/BoolField.test.tsx:
##########
@@ -109,3 +120,29 @@ test("<BoolField> - renders a wrapper with unknown props",
() => {
expect(screen.getByTestId("wrapper-field").getAttribute("data-y")).toBe("y");
expect(screen.getByTestId("wrapper-field").getAttribute("data-z")).toBe("z");
});
+
+test("<BoolField> - respects default false from model", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean }
}, { model: { x: false } }));
+
+ const input = screen.getByTestId("bool-field");
+ expect(input).not.toBeChecked();
+});
Review Comment:
It seems to be already checked on line 91.
We already have a test which checks if BoolField is unchecked without a
model plus a test to check if BoolField is checked when the model has value=true
```suggestion
```
##########
packages/runtime-tools-components/src/utils/ModelConversionTool.ts:
##########
@@ -96,6 +119,13 @@ function convertModel(model: any, schema: Record<string,
any>, ctx: ConversionCo
return;
}
+ if (value === undefined || value === null) {
Review Comment:
This code will not be executed as we have this before:
```
if (value === null) {
return;
}
```
##########
packages/runtime-tools-components/src/utils/ModelConversionTool.ts:
##########
@@ -96,6 +119,13 @@ function convertModel(model: any, schema: Record<string,
any>, ctx: ConversionCo
return;
}
+ if (value === undefined || value === null) {
+ if (propSchema.type === "boolean") {
Review Comment:
why the need of nested ifs?
##########
packages/runtime-tools-components/src/utils/ModelConversionTool.ts:
##########
@@ -25,6 +25,28 @@ export class ModelConversionTool {
public static convertStringToDate = (model: any, schema: Record<string,
any>): any => {
return doConvert(model, schema, (value) => new Date(value));
};
+
+ public static normalizeBooleans = (model: any, schema: Record<string, any>):
any => {
+ return doConvert(model, schema, (value) => {
+ if (value === "boolean") {
Review Comment:
Do we ever reach this code?
What did you mean with `value === "boolean"`?
##########
packages/uniforms-patternfly/tests/BoolField.test.tsx:
##########
@@ -77,6 +77,17 @@ test("<BoolField> - renders a input with correct value
(default)", () => {
expect(screen.getByTestId("bool-field")).not.toHaveAttribute("checked");
});
+test("<BoolField> - renders an input with correct value (default true)", () =>
{
+ render(
+ usingUniformsContext(<BoolField name="x" />, {
+ x: { type: Boolean, defaultValue: true },
+ })
+ );
Review Comment:
In the real use-case `defaultValue` in the schema is translated to `default`
by the backend.
So the frontend receive `default` in the `openapi.json` and what we need to
test is:
```
render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean }
}, { model: { x: true } }));
```
Which is already tested in line 92
##########
packages/uniforms-patternfly/tests/BoolField.test.tsx:
##########
@@ -109,3 +120,29 @@ test("<BoolField> - renders a wrapper with unknown props",
() => {
expect(screen.getByTestId("wrapper-field").getAttribute("data-y")).toBe("y");
expect(screen.getByTestId("wrapper-field").getAttribute("data-z")).toBe("z");
});
+
+test("<BoolField> - respects default false from model", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean }
}, { model: { x: false } }));
+
+ const input = screen.getByTestId("bool-field");
+ expect(input).not.toBeChecked();
+});
+
+test("<BoolField> - respects default true from model", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean }
}, { model: { x: true } }));
+
+ const input = screen.getByTestId("bool-field");
+ expect(input).toBeChecked();
+});
+
+test("<BoolField> - respects schema default true", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean,
defaultValue: true } }));
+ const input = screen.getByTestId("bool-field");
+ expect(input).toBeChecked();
+});
Review Comment:
It seems to be already checked on line 80
```suggestion
```
##########
packages/uniforms-patternfly/tests/BoolField.test.tsx:
##########
@@ -109,3 +120,29 @@ test("<BoolField> - renders a wrapper with unknown props",
() => {
expect(screen.getByTestId("wrapper-field").getAttribute("data-y")).toBe("y");
expect(screen.getByTestId("wrapper-field").getAttribute("data-z")).toBe("z");
});
+
+test("<BoolField> - respects default false from model", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean }
}, { model: { x: false } }));
+
+ const input = screen.getByTestId("bool-field");
+ expect(input).not.toBeChecked();
+});
+
+test("<BoolField> - respects default true from model", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean }
}, { model: { x: true } }));
+
+ const input = screen.getByTestId("bool-field");
+ expect(input).toBeChecked();
+});
Review Comment:
It seems to be already checked on line 91
```suggestion
```
##########
packages/runtime-tools-components/src/utils/ModelConversionTool.ts:
##########
@@ -25,6 +25,28 @@ export class ModelConversionTool {
public static convertStringToDate = (model: any, schema: Record<string,
any>): any => {
return doConvert(model, schema, (value) => new Date(value));
};
+
+ public static normalizeBooleans = (model: any, schema: Record<string, any>):
any => {
+ return doConvert(model, schema, (value) => {
+ if (value === "boolean") {
+ return !!value;
+ }
+ return value;
+ });
+ };
+
+ public static applySchemaDefaults(model: any, schema: Record<string, any>):
any {
+ const newModel = { ...model };
+ if (schema.properties) {
+ Object.keys(schema.properties).forEach((key) => {
+ const prop = schema.properties[key];
+ if (newModel[key] === undefined && prop.default !== undefined) {
Review Comment:
I think this function is in the right direction.
For booleans we should add a value even if there is no default value in the
schema
##########
packages/uniforms-patternfly/tests/BoolField.test.tsx:
##########
@@ -109,3 +120,29 @@ test("<BoolField> - renders a wrapper with unknown props",
() => {
expect(screen.getByTestId("wrapper-field").getAttribute("data-y")).toBe("y");
expect(screen.getByTestId("wrapper-field").getAttribute("data-z")).toBe("z");
});
+
+test("<BoolField> - respects default false from model", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean }
}, { model: { x: false } }));
+
+ const input = screen.getByTestId("bool-field");
+ expect(input).not.toBeChecked();
+});
+
+test("<BoolField> - respects default true from model", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean }
}, { model: { x: true } }));
+
+ const input = screen.getByTestId("bool-field");
+ expect(input).toBeChecked();
+});
+
+test("<BoolField> - respects schema default true", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean,
defaultValue: true } }));
+ const input = screen.getByTestId("bool-field");
+ expect(input).toBeChecked();
+});
+
+test("<BoolField> - respects schema default false", () => {
+ render(usingUniformsContext(<BoolField name="x" />, { x: { type: Boolean,
defaultValue: false } }));
+ const input = screen.getByTestId("bool-field");
+ expect(input).not.toBeChecked();
+});
Review Comment:
Repetition of other tests
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
