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

Distinguish empty arrays from an unset attribute #334

Open
migu0 opened this issue Aug 12, 2015 · 13 comments · May be fixed by #354
Open

Distinguish empty arrays from an unset attribute #334

migu0 opened this issue Aug 12, 2015 · 13 comments · May be fixed by #354

Comments

@migu0
Copy link

migu0 commented Aug 12, 2015

I have several forms that submit to a Virtus object (through a controller). Some forms contain an extras attribute, others don't.

I currently can't distinguish whether extras has been set to an empty array (i.e. the user deselects all checkboxes on the form) or whether extras has never been submitted. In either case, extras will be an empty array.

I need this distinction because if the user deselects all extras I need to update them. On the other hand, if extras were not part of the form (i.e. not in the params), I shouldn't update them.

    class UpdateForm
      include Virtus.model(nullify_blank: true)
      include ActiveModel::Model
      attribute :extras, Array
      attribute :booking_time, Time

      def save
        updatable_attributes = self.attributes.delete_if { |key, value| value.blank? }
        some_object.update(updatable_attributes)
      end
    end

How can I make Virtus to give me a nil on extras if I call it like this:

    UpdateForm.new(booking_time: Time.current)

Or is there a better pattern to do this?

@solnic
Copy link
Owner

solnic commented Aug 12, 2015

You can do attribute :extras, Array, coerce: false so that nil won't be coerced to an empty array

@migu0
Copy link
Author

migu0 commented Aug 12, 2015

Thanks for your reply.

I tried that just now. So no extras were passed into the Virtus model, attribute :extras, Array, coerce: false is set like you said but if I call self.attributes I still get :extras=>[]

@forest
Copy link

forest commented Feb 12, 2016

@michaelimstepf I'm having the same issue. Did you find a solution to this?

@migu0
Copy link
Author

migu0 commented Feb 13, 2016

@forest Sorry, pls ignore previous comment (deleted), I was referring to a different issue. Can you try this:

class UpdateForm
  include Virtus.model(nullify_blank: true)
  include ActiveModel::Model
  attribute :extras, Array
  attribute :booking_time, Time

  def initialize(opts={})
    super
    # http://stackoverflow.com/a/31959720/1076279      
    @extras = nil unless opts['extras'].present?
  end

  def save
    some_object.update(updatable_attributes)
  end
end

@forest
Copy link

forest commented Feb 13, 2016

@michaelimstepf thanks for the reply. If I just needed this for one attribute on one class I'd go this route.

I opted for this https://gist.github.com/657fd558a9c05416cf81 temporary solution until #209 lands on master so I can create my own attribute extension.

It is odd to me that NullifyBlank only works on strings, but I wasn't sure if others would agree.

@migu0
Copy link
Author

migu0 commented Feb 14, 2016

Thanks for posting this

@mherold
Copy link

mherold commented Feb 29, 2016

This (kind of) worked for me:
attribute :extras, Array, coerce: false, default: nil

But ;-) If you look at the use case "I want to be able to distinguish nil from empty", I don't think any of the currently suggested solutions/workarounds are totally fitting:

  • With the solution above, you loose coercion functionality (i.e. you could set the attribute's value to types you might not want)
  • When overwriting the initialize method only works for initialization (obviously ;-), but you can't set the attribute to nil later on
  • Overwriting NullifyBlank like in https://gist.github.com/forest/657fd558a9c05416cf81 makes it impossible to set the attribute to an empty array (because it's blank it gets nullified)

I would expect that specifying required: false would also work on Array types in the way that I can assign both nil and an empty array to the attribute.

Since nil gets coerced to an empty Array by the coercer, maybe the code to adapt is Virtus::Attribute::Coercible#set, like this:

      def set(instance, value)
        coerced_value = value.nil? && !required? ? value : coerce(value)
        super(instance, coerced_value)
      end

(edit:) With this change, setting required: false implies that nil should never be coerced, regardless of the type, even if it could be.

I can make a PR and write some tests if that's an acceptable solution, what do you think?

@forest
Copy link

forest commented Feb 29, 2016

@mherold your analysis and solution sounds right. I knew changing NullifyBlank wasn't the right solution. :)

I'm happy to test with my use case when you push up the PR.

@mherold
Copy link

mherold commented Mar 1, 2016

Thanks @forest, I appreciate it!! PR is available.

@forest
Copy link

forest commented Mar 3, 2016

@mherold PR #354 covers my use case. I was able to remove my hack to NullifyBlank.

@mherold
Copy link

mherold commented Mar 6, 2016

@forest awesome, thanks for testing!

@ka8725
Copy link

ka8725 commented Apr 29, 2016

@forest we have such hack in our app either. It would be great to have it merged.

@fguillen
Copy link

fguillen commented Apr 6, 2017

Is it possible to fix this? is there any problem with @mherold PR #354 suggestion?

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

Successfully merging a pull request may close this issue.

6 participants