Skip to content

[Bug]: ValidationTypes.Number returning 'min' value (when min value validation is set) instead of the parsed value #17472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
1 task done
iamrkcheers opened this issue Oct 11, 2022 · 7 comments · Fixed by #18001
Assignees
Labels
Bug Something isn't working Needs Triaging Needs attention from maintainers to triage Verified When issue is retested post its fixed Widget Validation Issues related to widget property validation Widgets Product This label groups issues related to widgets

Comments

@iamrkcheers
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Description

When the 'min' value validation is set, eg:
validation: { type: ValidationTypes.NUMBER, params: { min: 1 }, }
The expected value is supposed to be the parsed value.
However, what is being returned is the minimum value.

Steps To Reproduce

  1. Open the 'index.tsx' file for any widget having a validation of the type 'Number' for any of its properties. eg: 'maxNumFiles' property for the 'FilePicker' widget.
  2. Add a validation to set its minimum value. eg:
    validation: { type: ValidationTypes.NUMBER, params: { min: 1}, }
  3. Enter 0 for the 'Max No. of files' property in the canvas for the widget.
  4. Debug to find the value returned. eg: in our case, 'this.props.maxNumFiles' returns the min value ie 1, instead of the evaluated value ie 0.

Public Sample App

No response

Version

Cloud - 1.8.5

@iamrkcheers iamrkcheers added Bug Something isn't working Needs Triaging Needs attention from maintainers to triage Javascript Product Issues related to users writing javascript in appsmith labels Oct 11, 2022
@github-actions github-actions bot removed the Javascript Product Issues related to users writing javascript in appsmith label Oct 11, 2022
@somangshu somangshu added the Widget Validation Issues related to widget property validation label Oct 11, 2022
@github-actions github-actions bot added the Widgets Product This label groups issues related to widgets label Oct 11, 2022
@somangshu
Copy link
Contributor

@riodeuno can you help me a bit here? Wondering if this is expected? Or should we go about identifying how to make the change?

@riodeuno
Copy link
Contributor

The rationale behind having min and max here, is that the values beyond the bounds would break the application or have the widget work incorrectly. This is why the fallback is the min value.

I have been thinking about a facility to have the value simply passthrough if the value should always be the parsed value. I can see how this can be needed.

Tentative thought:
Have another parameter called "passthrough" : true for validation types, this will ignore any validation safety and pass the parsed value to the widget, even if it fails validation.

Alternatively, if we're confident in all our widgets, we can remove all validation safety mechanisms, and have the value always passthrough as parsed value.

@sbalaji1192
Copy link
Contributor

@riodeuno the validation logic of min of Number validation already passThrough. But there is an issue in the expression that passes all parsed values except 0.

return {
     isValid: false,
     parsed: parsed || config.params.min || 0,
     messages: [`Minimum allowed value: ${config.params.min}`],
};

When the parsed value is 0 it returns the min value. but for all other values, it returns the parsed value.

@iamrkcheers
Copy link
Author

@aswathkk
Copy link
Contributor

@iamrkcheers

As @sbalaji1192 mentioned, the passthrough behaviour won't work if the value is 0. In those cases, the returned value becomes the min value provided in the configuration.

The way to fix this behaviour is to change parsed || config.params.min || 0 to parsed ?? config.params.min ?? 0.
If we do that, existing applications might break wherever we have configured the min value to be greater than 0. In my exploration, the following properties might break in the existing applications.

  • FilePickerWidget: maxFileSize
  • FilePickerWidgetV2: maxFileSize
  • InputWidgetV2: maxChars
  • ProgressBarWidget: steps
  • ProgressWidget: steps

We need to handle this in such a way that the behaviour of these properties is maintained. May be like @riodeuno told, we can introduce a boolean parameter called passthrough and set it as false for the above mentioned properties.

@aswathkk
Copy link
Contributor

@sbalaji1192 Yeah. Then this parameter should be there only for NUMBER type validation. Let's do it like that then.

@aswathkk aswathkk assigned aswathkk and unassigned riodeuno and somangshu Oct 28, 2022
@close-label close-label bot added the QA Needs QA attention label Nov 1, 2022
@kamakshibhat-appsmith
Copy link

as discussed with @aswathkk ,this cannot be tested . However, just tested the existing functionality. seems fine

@kamakshibhat-appsmith kamakshibhat-appsmith added Verified When issue is retested post its fixed and removed QA Needs QA attention labels Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Needs Triaging Needs attention from maintainers to triage Verified When issue is retested post its fixed Widget Validation Issues related to widget property validation Widgets Product This label groups issues related to widgets
Projects
None yet
Development

Successfully merging a pull request may close this issue.