appsmith icon indicating copy to clipboard operation
appsmith copied to clipboard

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

Open iamrkcheers opened this issue 2 years ago • 6 comments

Is there an existing issue for this?

  • [X] 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 avatar Oct 11 '22 07:10 iamrkcheers

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

somangshu avatar Oct 11 '22 08:10 somangshu

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.

riodeuno avatar Oct 11 '22 11:10 riodeuno

@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.

sbalaji1192 avatar Oct 11 '22 11:10 sbalaji1192

@aswathkk @rohitagarwal88 Could you help us out ? The issue [Bug]-[130]:File picker Widget - able to pick a file when Max no of files is defined as zero (0) #14857 is being blocked by this.

iamrkcheers avatar Oct 13 '22 11:10 iamrkcheers

@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 avatar Oct 18 '22 05:10 aswathkk

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

aswathkk avatar Oct 18 '22 05:10 aswathkk

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

kamakshibhat-appsmith avatar Nov 18 '22 05:11 kamakshibhat-appsmith