appsmithorg / appsmith

Platform to build admin panels, internal tools, and dashboards. Integrates with 25+ databases and any API.
https://www.appsmith.com
Apache License 2.0
33.77k stars 3.63k forks source link

[Bug]: Filepicker widget does not validate size when modified #9245

Open ramsaptami opened 2 years ago

ramsaptami commented 2 years ago

Is there an existing issue for this?

Current Behavior

Filepicker widget does not validate number of files and file size when it's been updated to a lower value and continues without throwing an error

LOOM DEMO

Steps To Reproduce

  1. Drag and drop a filepicker widget and upload files and sizes equal to the one mentioned on the property pane
  2. Reduce the size and retry file upload with the same files as before
  3. Observe that the number of files and size are not validated with new values

Environment

Production

Version

Cloud

somangshu commented 2 years ago

@ramsaptami we have a fixed agenda for the epic property pane improvements, this does not fall under the scope. Also the issue does not effect the end-user and is an edge case which is really not causing any breakage, deprioritizing this

abhvsn commented 2 years ago

@somangshu can this be related to https://app.intercom.com/a/apps/y10e7138/conversations/164629100057109?redirectTo=feed if yes then I think we should increase the priority to high or even critical as user had really bad experience with video upload

somangshu commented 2 years ago

For more context from the user as per @abhvsn

User is facing the below error:

"File content is not base64 encoded. File content needs to be base64 encoded when the 'File Data Type: Base64/Text' field is selected 'Yes'. "

The user is trying to add a MP4 video file above 5 MB. They are trying to upload with the base64 format

The user has also tried changing the max file size property to 25Mb.

We need to figure if this is possible today. If not what should be the resolution here. cc @SatishGandham

somangshu commented 2 years ago

Marked critical until we arrive a solution.

SatishGandham commented 2 years ago

@somangshu , I tried a 33MB mp4 file with base64 format. It is working fine on my dev (which is 6 days old), but is failing on release even before making the execute call. could be a regression?

Did something change in between?

From what the user is describing, looks like he did not select the correct data type in file picker widget. @abhvsn , can you confirm that?

somangshu commented 2 years ago

@SatishGandham are you able to see a change in the release environment that could be causing this ?

somangshu commented 2 years ago

@vivekverma2312 this is a regression, Can you test and confirm the above?

vivekverma2312 commented 2 years ago

@somangshu Tested and issue is reproducible, marking it as release blocker.

ramsaptami commented 2 years ago

I'm not sure if this is a regression that we saw recently. Seen this happen for quite sometime now

somangshu commented 2 years ago

@ramsaptami Satish mentioned that this is working in the local dev.

Ill try and allocate some bandwidth to understand more.

somangshu commented 2 years ago

@ashit-rath if you have some bandwidth, can you try to find the root cause.

dilippitchika commented 2 years ago

The problem I was able to reproduce - When you upload files in a filepicker and then change the no.of files or file size. The existing files uploaded are not revalidated.

Real scenario where this is a problem - If the max files and file size is controlled using another widget in the app. Like if you are allowing users to control how many images they upload and size of images, then this flow is confusing for the app viewers. Very rare flow in itself.

I don't have the right solution but possible idea for this is Whenever the max files or max file size changes - Discard all existing files and reset the widget

@somangshu @techbhavin - I don't think we should work on this now. If agreed, I'll move this to icebox.

somangshu commented 2 years ago

I like the solution you are proposing. Not a burning problem according to me right now because the app viewer cant be directly affected by this since these are not really JS convertible to have dynamic values.

Another point with resetting the data on these attribute updates is that in case in the future we allow this value to be dynamic, To the app viewer / end user the behaviour might be unpredictable as there is no way for them to understand where there uploaded files go when there is an update in the max file / max size property

the implementation is easy but the consequences need to be thought through. In favour of that I suggest we move this to the ice box

ramsaptami commented 2 years ago

Another point with resetting the data on these attribute updates is that in case in the future we allow this value to be dynamic, To the app viewer / end user the behaviour might be unpredictable as there is no way for them to understand where there uploaded files go when there is an update in the max file / max size property

Would it make sense to add a message of some sort after re-evaluation to let the user know the reason why uploaded files have been cleared?

somangshu commented 2 years ago

@ramsaptami I am still of the opinion that it would not be a good experience for the end user, until they are aware of this already. Since its a one way road, and the app viewer dosent really have anything under there control, the additional overhead will be to let the app dev know that there user will see this Behavior on a certain use case