solnic / virtus

[DISCONTINUED ] Attributes on Steroids for Plain Old Ruby Objects
MIT License
3.77k stars 228 forks source link

Distinguish empty arrays from an unset attribute #334

Open migu0 opened 8 years ago

migu0 commented 8 years ago

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 commented 8 years ago

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

migu0 commented 8 years ago

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 commented 8 years ago

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

migu0 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

Thanks for posting this

mherold commented 8 years ago

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:

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 commented 8 years ago

@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 commented 8 years ago

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

forest commented 8 years ago

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

mherold commented 8 years ago

@forest awesome, thanks for testing!

ka8725 commented 8 years ago

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

fguillen commented 7 years ago

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