Fix cryptic file validation errors by ahukkanen · Pull Request #9663 · decidim/decidim

ahukkanen · 2022-08-07T11:42:47Z

🎩 What? Why?

Currently the file validation messages are rather cryptic for the general user because they are showing the content type matching pattern regular expressions converted into strings.

To make it understandable for the general public, we want to show the file extensions instead of the content types.

📌 Related Issues

Fixes Cryptic file validation error #9185

Testing

Enable attachments for proposals
Try to upload an attachment which is not allowed, e.g. a .zip file is not allowed by default
See a better error message

andreslucena · 2022-09-15T14:37:29Z

This is a great improvement on what we have for sure!

I think we can tune it a bit more. Now it says "file should be one of *.bmp, *.gif, *.jpeg, *.jpg, *.pdf, *.png, *.rtf, .txt". Even though the second part is much better, it still sounds a bit too technical. The first part ("should be one of") sounds like Set Theory, and the second part (.bmp) has regular expressions. Could you change it to something like this 🙏🏽?

Only files with the following extensions are allowed: gif jpeg jpg png bmp

In the avatar file upload in the participant account, there's something that breaks this:

As far as I could find is the only one with mixed formats.

In the file upload attachment of a participatory process, the file extensions of the error doesn't correspond with the ones from the help text.

On the private participants census upload CSV it says text/csv

(Mind that the help text doesn't correspond and talks about images, but that's out of the scope of the current PR)

Previously this only applied to the validators run for files attached through the uploaders. Which is problematic e.g. for CSV files that we just upload and process without an uploader in-between.

Previously it was always using the defaults instead of the settings configured for the target organization attached to the record.

The votings census file upload broke after modifying the file validation logic as it was missing the current participatory space where it fetches the organization from.

In order to fetch the correct file extensions in each situation for the dynamic upload errors, we need to define a proxy record to pass the correct upload context for the validations.

In order to show the file extensions similarly in the help texts and the errors, sort them at the humanizer which produces the extensions list for the help texts.

ahukkanen · 2022-09-20T14:50:16Z

@andreslucena All issues fixed that I could fix in this PR. The rest will be fixed by another PR #9623. See explanations below.

I think we can tune it a bit more. Now it says "file should be one of *.bmp, *.gif, *.jpeg, *.jpg, *.pdf, *.png, *.rtf, .txt". Even though the second part is much better, it still sounds a bit too technical. The first part ("should be one of") sounds like Set Theory, and the second part (.bmp) has regular expressions. Could you change it to something like this 🙏🏽?

Only files with the following extensions are allowed: gif jpeg jpg png bmp

Done. Also changed the other similar error messages to match this format.

In the avatar file upload in the participant account, there's something that breaks this:

As far as I could find is the only one with mixed formats.

The mixed format is because Decidim produces unknown MIME types currently. E.g. image/bmp is not known for MiniMime (instead, it knows image/x-bmp). And also image/jpg is not a valida MIME type anywhere.

This has been a persistent issue in Decidim for years but I am fixing it in another PR #9623.

In particular it is this line that will fix this issue:

decidim/decidim-core/app/uploaders/decidim/image_uploader.rb

Line 11 in 4b9d539

    
           extension_allowlist.map { |ext| MiniMime.lookup_by_extension(ext).content_type }.uniq

Do note that this same issue will still persist in case the system administrator has configured MIME types unknown for MiniMime. But I would see that as misconfiguration rather than a bug, and it'd be good to show those "extra" unknown MIME types for the user in case that happens to make it easier to identify potential issues regarding those.

In the file upload attachment of a participatory process, the file extensions of the error doesn't correspond with the ones from the help text.

This was because the file validation wasn't context aware whether the validation is happening at the admin panel or the participant side of the system which can have different allowed extensions through the system settings.

I have fixed this issue in the latest revisions.

On the private participants census upload CSV it says text/csv

This was because initially I only applied these changes to the upload validations happening for files that are attached to the records through uploaders. This was not applied in case you just used validates :some_temp_file, file_content_type: { allow: ["text/csv"] } which happens e.g. for the described use case (votings census).

I had to do a bit of a "hack" to also fix the messages for such direct validations by overriding one method from ActiveModel::Validations::FileContentTypeValidator. But anyways, now it should work for both cases.

andreslucena · 2022-09-30T09:18:05Z

The mixed format is because Decidim produces unknown MIME types currently. E.g. image/bmp is not known for MiniMime (instead, it knows image/x-bmp). And also image/jpg is not a valida MIME type anywhere.

This has been a persistent issue in Decidim for years but I am fixing it in another PR #9623.

Great! I'll check that one after merging this one.

Do note that this same issue will still persist in case the system administrator has configured MIME types unknown for MiniMime. But I would see that as misconfiguration rather than a bug, and it'd be good to show those "extra" unknown MIME types for the user in case that happens to make it easier to identify potential issues regarding those.

Ok, I agree.

andreslucena

I've rechecked all my comments and are all fixed 👍🏽 👍🏽
Code-wise everything is great, documented, and tested. 💯

* feature/redesign-turbo: Install turbo-rails Fix incorrect expecations after change of allowed file extensions (#9877) Update `@joeattardi/emoji-button` to `@picmo/popup-picker` (#9667) Address Crowdin feedback (#9678) Fix cryptic file validation errors (#9663) Fix disappearing sub-lists in rich text editors (#9867) Implement authorization data recovery for deleted users (#9463) Redesigned: admin bar (#9874)

* Fix cryptic file validation errors * Improve the content type validator and add method documentation * Add content type validator spec for denylist case * Fix the file definition within the spec * Fix specs broken because of the message changes * Update the error message for invalid file content type as per review * Fix the obfuscated error messages also for file content type validators Previously this only applied to the validators run for files attached through the uploaders. Which is problematic e.g. for CSV files that we just upload and process without an uploader in-between. * Add spec for the customized file content type validator * Get the extension allowlist through the uploader if available * Fix the extension_allowlist translation * Move the extension allowlist translation to the correct module * Fetch the extension allowlist through the correct recrod Previously it was always using the defaults instead of the settings configured for the target organization attached to the record. * Pass the context to the form records assigned to PassthruValidator The votings census file upload broke after modifying the file validation logic as it was missing the current participatory space where it fetches the organization from. * Pass the correct upload context for the dynamic file validations In order to fetch the correct file extensions in each situation for the dynamic upload errors, we need to define a proxy record to pass the correct upload context for the validations. * Sort the file extensions at the FileValidatorHumanizer In order to show the file extensions similarly in the help texts and the errors, sort them at the humanizer which produces the extensions list for the help texts. * Fix file not attached to organization with attachment proxy * Normalize locales * Use delegate method instead of extending SimpleDelegator

Fix cryptic file validation errors

7fb5a31

ahukkanen added module: proposals module: core type: fix PRs that implement a fix for a bug labels Aug 7, 2022

ahukkanen added 4 commits August 7, 2022 15:33

Improve the content type validator and add method documentation

e780d51

Add content type validator spec for denylist case

3b9a919

Fix the file definition within the spec

7e4b467

Fix specs broken because of the message changes

b184e78

ahukkanen marked this pull request as ready for review August 7, 2022 13:18

ahukkanen added 11 commits September 20, 2022 11:09

Merge branch 'develop' into fix/9185

2e383b7

Update the error message for invalid file content type as per review

a9275ba

Fix the obfuscated error messages also for file content type validators

c3bcaf5

Previously this only applied to the validators run for files attached through the uploaders. Which is problematic e.g. for CSV files that we just upload and process without an uploader in-between.

Add spec for the customized file content type validator

58cc9e6

Get the extension allowlist through the uploader if available

cc28b37

Fix the extension_allowlist translation

035aad8

Move the extension allowlist translation to the correct module

c421542

Fetch the extension allowlist through the correct recrod

1bb146b

Previously it was always using the defaults instead of the settings configured for the target organization attached to the record.

Pass the context to the form records assigned to PassthruValidator

b32b897

The votings census file upload broke after modifying the file validation logic as it was missing the current participatory space where it fetches the organization from.

Pass the correct upload context for the dynamic file validations

2c5e217

In order to fetch the correct file extensions in each situation for the dynamic upload errors, we need to define a proxy record to pass the correct upload context for the validations.

Sort the file extensions at the FileValidatorHumanizer

0d57a04

In order to show the file extensions similarly in the help texts and the errors, sort them at the humanizer which produces the extensions list for the help texts.

ahukkanen added 3 commits September 20, 2022 18:05

Fix file not attached to organization with attachment proxy

8516aff

Normalize locales

aff05ba

Use delegate method instead of extending SimpleDelegator

c95497e

andreslucena approved these changes Sep 30, 2022

View reviewed changes

andreslucena merged commit df67cd5 into decidim:develop Oct 4, 2022

ahukkanen mentioned this pull request Oct 4, 2022

Fix incorrect expecations after change of allowed file extensions #9877

Merged

ahukkanen deleted the fix/9185 branch October 4, 2022 13:02

andreslucena mentioned this pull request Oct 26, 2022

Backport 'Fix cryptic file validation errors' to v0.27 #9971

Merged

alecslupu mentioned this pull request Jul 19, 2023

Analize the PRs in the last year to consider new backports. #11233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix cryptic file validation errors#9663

Fix cryptic file validation errors#9663
andreslucena merged 19 commits intodecidim:developfrom
mainio:fix/9185

ahukkanen commented Aug 7, 2022

Uh oh!

andreslucena commented Sep 15, 2022

Uh oh!

ahukkanen commented Sep 20, 2022 •

edited

Loading

Uh oh!

andreslucena commented Sep 30, 2022

Uh oh!

andreslucena left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ahukkanen commented Aug 7, 2022

🎩 What? Why?

📌 Related Issues

Testing

Uh oh!

andreslucena commented Sep 15, 2022

Uh oh!

ahukkanen commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andreslucena commented Sep 30, 2022

Uh oh!

andreslucena left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ahukkanen commented Sep 20, 2022 •

edited

Loading