Skip to content
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

Passing :allow_blank option raises #243

Closed
joelmoss opened this issue Feb 27, 2024 · 8 comments · Fixed by #245
Closed

Passing :allow_blank option raises #243

joelmoss opened this issue Feb 27, 2024 · 8 comments · Fixed by #245
Assignees

Comments

@joelmoss
Copy link

I am trying to allow a blank attachment during validation...

has_one_attached :photo
validates :photo, attached: true, on: %i[create update], allow_blank: true,
                    content_type: { in: %r{\Aimage/.*\z}, message: :image_content_type_invalid },
                    size: { less_than: 5.megabytes }

But it fails with...

ArgumentError: You cannot pass the :allow_blank option to this validator (ArgumentError)

I can see in the code that the allow_blank option is supported, so why does this not work?

thankyou

@Mth0158
Copy link
Collaborator

Mth0158 commented Feb 28, 2024

Hi @joelmoss
The error comes from the attached validator (https://github.com/igorkasyanchuk/active_storage_validations/blob/master/lib/active_storage_validations/attached_validator.rb). It does not accept the allow_blank option when activated, since it requires to attach. It would be contradictory to require to attach something and to allow not to attach something, therefore the error.
Maybe the solution is to remove the attached: true validator?

@Mth0158
Copy link
Collaborator

Mth0158 commented Mar 12, 2024

I am pushing a quick MR to make the message clearer for future users

Mth0158 added a commit that referenced this issue Mar 13, 2024
…ption-raises

[Validator] Better error message when using :allow_blank/nil with attached validator (#243)
@Polo2
Copy link

Polo2 commented Apr 24, 2024

👋 here, and thanks for the gem.

I'm also wondering about allowing optional attachement and I went to this issue.
I understand with your answer that removing attached: true would work to keep attachement optional.

But it's not very easy to read and guess for other developpers:

for example with this line:
validates :logo, content_type: [...]
it's not super clear that logo is optional.

Instead of adding a comment in our code everywhere optional attachements are validated,
Would it work too by adding explicit attached: false, or attached: nil?

Or should I go to
validates :logo, content_type: [...], allow_blank: true
(no attached validator, but allow_blank allowed in this case?)

thanks in advance for your answer, if you want I can open a tiny PR on ready to highlight this usecase.

@Mth0158
Copy link
Collaborator

Mth0158 commented Apr 24, 2024

Hi from Lyon @Polo2 👋

We could develop something like attached: false, but I think it's not the best option here.

ActiveStorage has_one_attached / has_many_attached association methods, like ActiveRecord has_one and have_many, by default allow to have empty child / children relation. It implicitly "implies" the allow_nil / allow_blank options.

You could write your association with one of these to make you code more explicit. All the gem validators, expect the attached one, allow these options (check our tests at https://github.com/search?q=repo%3Aigorkasyanchuk%2Factive_storage_validations+WorksWithAllRailsCommonValidationOptions&type=code).

I think it's by far the simplest & clearer option, but I am open to discussion :)

@Mth0158 Mth0158 reopened this Apr 24, 2024
@Mth0158 Mth0158 closed this as completed Jul 1, 2024
@amit
Copy link

amit commented Sep 6, 2024

I got hit by this today after I upgraded to newer version of this gem.
How do I specify that an attachment is optional now?

This was my code before:

  has_one_attached :profile_picture
  validates :profile_picture, attached: true, allow_blank: true, content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif", "image/webp"]

How should this be modified with the newer version? Should I just drop attached: true?

@Mth0158
Copy link
Collaborator

Mth0158 commented Sep 7, 2024

Hi @amit!
Looking at your code, you both require an attachment attached: true and allow no attachment allow_blank: true. If you want to make attachment optional, you have have to remove the attached: true. Plus, you could also remove the allow_blank: true, by default ActiveStorage allows no attachments, so the option is more or less useless :)

@amit
Copy link

amit commented Sep 7, 2024

Thanks! So the new code then should be:

  has_one_attached :profile_picture
  validates :profile_picture, content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif", "image/webp"]


Correct? This allows no attachment as valid, but if it is attached, then it needs to comply with the content_type given?

The documentation of attached does not specify what it really means and all examples seem to show the value of true!

@Mth0158
Copy link
Collaborator

Mth0158 commented Sep 8, 2024

@amit that's exactly it! Your code does what you explained.
I do agree with you, the readme could be rewritten to explain the available options for each validator, it's not the clearest right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants