trailblazer / reform

Form objects decoupled from models.
https://trailblazer.to/2.1/docs/reform.html
MIT License
2.49k stars 184 forks source link

Any way to handle accepts_nested_attributes_for-like API? #86

Closed Azdaroth closed 9 years ago

Azdaroth commented 10 years ago

Is there any neat way to handle such things as :reject_if or _destroy parameter like accepts_nested_attributes_for does?

I have some pretty ugly code to handle removing elements from collection when _destroy parameter is set (conditional validation which happens only when _destroy == "false" and overriding #sync_models method with rejecting elements with where _destroy != "false).

My use case is: I have a form, where you can add more elements to parent model (with jquery_nested_form), the problem starts when a person e.g. by mistake clicks on "Add more" or started filling new form and decided that the item is not needed. In first case I'd like to prevent instantiating new model because all values are blank and in second case, instead of removing content from inputs, just click on "remove", which causes _destroy parameter to be set.

I tried playing with populator but couldn't achieve anything interesting. Mixing form objects with accepts_nested_attributes_for is not a good idea and using accepts_nested_attributes_for alone is not an option (because of some virtual properties etc.). Any idea how to solve that problem in a better way?

apotonick commented 10 years ago

A quick way to achieve this is override #validate and just delete the unwanted items from params.

I'd love to see examplary incoming params and then a short explanation what should happen in the form. Maybe we can sort that generically. Thanks!!!

Azdaroth commented 10 years ago

Here's a example of params:

 Parameters: {
  company_client"=>{
    # many params
    "related_clients_attributes"=>{
      "1400315121055"=>{
        # tons of params
        "_destroy"=>"false"
         # or "_destroy" => 1
      }, 
    }
  }

And several things might happen here:

  1. If "_destroy" param is set to true (or 1) and there's no id param (so the record is not the persisted), it should not be populated.
  2. It would be quite convenient to skip population if some conditions are met, like reject_if: :all_blank in accepts_nested_attributes_for.
  3. If id param is present (the record is persisted), it should be destroyed while saving if _destroy param is set.

I'm not that familiar with internals of Reform / Representable, but can it be done in Reform alone? Maybe the API could like the one from accepts_nested_attributes_for:

collection :name, 
  reject_if: lambda { |fragment, args| # own strategy or just provide symbol like :all_blank for built-in way to handle such cases },
 allow_destroy: true

There's also one more thing I don't quite undestrand, I have a #persistable method which runs validation and sync_models if everything is ok (some pretty complex usecase, where service object knows more about dealing with persistence so I don't use Reform#save):

def persistable?(params)
    if validate(params)
      sync_models
      true 
    end
  end

I have a client model with related_clients (a bit like parent - children relation):

model :client

collection :related_clients

Before validation, model.related_clients returns empty collection, but after validation (and before syncing models) the collection is not empty (e.g. 2 related clients were submitted and it returns 2 clients). Shouldn't this happen while syncing models?

apotonick commented 10 years ago

Thanks Karol, that was indeed a really good explanation, totally see my design "flaws" (missing features) now.

I'll think about the destroy stuff, as a form becomes more and more the API endpoint (especially in a Trailblazer architecture) this makes sense.

Reg. your second point - that is some hack I did to fix associations when populating. You're right that probably shouldn't happen in #validate but later. I'll look into it.

apotonick commented 10 years ago

Can you give me a list of the configurations from accepts_nested_attributes? If they make life easier, we can adopt them, this can all be done with Representable (which does all the data transformation in Reform).

@elvanja Isn't that something you might be interested in as well, as to our reform example?

Azdaroth commented 10 years ago

Configuration options: reject_if, allow_destroy, limit (does what is says, limits the number records that can be processed, can also be a lambda) and update_only (for one-to-one associations only, either update the existing record or replace with new one, but I've never used this one, don't really know how valuable is that feature).

Azdaroth commented 10 years ago

Nick, any chance you will be working on that feature in Representable soon? Maybe I could try implementing it, it's going to be pretty important in project I'm working on and it would be better to provide some generic solution than writing ugly code to handle such use cases.

apotonick commented 10 years ago

Yes, I want to think about that stuff. The only "problem" is that this kind of configurations should go in an API media type definition, such as Object-HAL which I'm working on.

Azdaroth commented 10 years ago
  1. reject_if is useful when you give possibility to add some items to collection, build (e.g. model.items.build) an item and don't want to process it if the form was untouched (like :all_blank strategy) - the simplest use case is when you can add several photos to gallery, the input is displayed (because the photo was pre-built), but you don't want to add any photo, so the blank params are sent - it should not be processed.
  2. Yeah, for unpersisted records (without id param) it should reject them, for persisted records it should destroy them (but is sounds more like an ActiveRecord part of Reform).
  3. Never really used that one, but maybe for some limitations like: you don't want to have more than 10 records in relation but I think you we can validate the length of collection in Reform's validations, so that's probably already solved.
elvanja commented 10 years ago

Hi guys, a bit late to the party... Anyway, the mapper/model manager approach has kind of grown on me. So I am using accepts_nested_attributes in a very limited manner (just as a way to white list nested parameters for easier usage of model manager - so I don't have to use permit explicitly). But, it might be a feature of interest for reform example....

apotonick commented 10 years ago

I did some research and thinking on this.

This will definitely go into representable's Object-HAL implementation, but I see the point that it could be helpful in a standard form.

  1. Not sure but isn't :if what you're after with the :reject_if option?

    property :album, if: lambda { |fragment| fragment["name"].present? }

    This would ignore an incoming hash for album: if it's name field was blank. If blank, the album property is simply skipped in validate.

  2. I can see the _destroy/allow_destroy purpose now. It's kinda the reverse case of :populate_if_emtpy.
apotonick commented 10 years ago

AAAaaah, I understand :reject_if now. It means a nested property (e.g. album) should be skipped in validate when its incoming hash is :all_blank or when the lambda is true.

Otherwise, Reform would assign empty strings to the properties of album and that would make the form invalid, right????

Azdaroth commented 10 years ago

Not sure about if:, I couldn't achieve anything interesting with it (no member 'name' in struct, I had only access to binding, represented, decorator etc.) and it wouldn't be that convenient to use as :all_blank (but that's just a strategy).

But you probably got it right, basically if lambda is true, the validation (and the population) should be skipped for that one.

apotonick commented 10 years ago

Hi Karol, I just wanted to let you know that I'm currently working on the problem that #validate writes to the model when :populate_if_empty is being used. This will all got out with Reform/Representable 2.0.

Azdaroth commented 10 years ago

Hi Nick, that's great! I'm really looking forward to 2.0 version. When can it be expected?

apotonick commented 10 years ago

Ah, you're gonna love it!

It will also have

Most of the stuff working on master already. Gimme a week.

apotonick commented 10 years ago

It actually all went into 1.1. Your one problem is fixed, where #validate would write to the model. This no longer happens!

I am currently working on reject_if:.

Azdaroth commented 10 years ago

That change in 1.1 was damn useful, I could remove some voodoo magic in my code ;).

apotonick commented 10 years ago

Same here, did you see amount of code that I could delete from validate.rb? Felt good! :hamburger:

apotonick commented 10 years ago

We now have skip_if: in Reform, @Azdaroth: https://github.com/apotonick/reform/commit/fa58198acf2d5b471888458b2667467db1a71188 (needs representable/master). :fireworks:

Azdaroth commented 10 years ago

Hell yeah, that's awesome. :metal:

Azdaroth commented 9 years ago

Hi Nick, have you been working on allow_destroy-like feature? If not, maybe I could do something with that? I think it may be pretty convenient stuff, I use it in several projects and it would be awesowe if Reform could do that instead of including custom modules.

This is my current solution: let's say that User has many addresses so we will have 2 forms: UserForm and AddressForm and the UserForm looks like this:

class UserForm < Reform::Form

  model :user

  # properties

  collection :address, populate_if_empty: lambda { |fragment, args| 
    user: model.addresses.build
  }, form: AddressForm

end

To handle deleting elements from collection withs _delete flag I use 2 modules: Reform::NestedFormParent which is included in parent form which overwrites save! and update! methods and gives ability to whitelist deletable collections and Reform::Deletable which is included in collection's form, in this case AddressForm. Here is the code:

module Reform::Deletable

  extend ActiveSupport::Concern

  included  do
    property :id, virtual: true
    property :_destroy, virtual: true
  end

  def marked_for_destruction?
    _destroy != "false"
  end 

  def not_persisted?
    id.blank?
  end

  def destroy
    model.destroy
  end

end
module Reform::NestedFormParent

  extend ActiveSupport::Concern

  NestedCollectionOptionsAggregate = Struct.new(:name)

  included do

    def self.deletable_collections
      @deletable_collections ||= []
    end

    def self.deletable_collection(name, options={})
      deletable_collections << NestedCollectionOptionsAggregate.new(name)
    end

  end

  def update!(params)
    super
    self.class.deletable_collections.each do |deletable_collection|
      filtered_collection = send(deletable_collection.name).reject do |element|
        element.marked_for_destruction? && element.not_persisted?
      end
      self.send("#{deletable_collection.name}=", filtered_collection) 
    end
  end

  def save!
    super
    # damn ugly, requires eliminating respond_to? calls and should be recursive
    if self.class.respond_to?(:deletable_collections)
      self.class.deletable_collections.each do |deletable_collection|
        send(deletable_collection.name).reject(&:marked_for_destruction?)
        .select { |el| el.class.respond_to?(:deletable_collections) }.each do |element|
          element.class.deletable_collections.each do |childs_deletable_collection|
            element.send(childs_deletable_collection.name).select(&:marked_for_destruction?).each(&:destroy)
          end
        end
        send(deletable_collection.name).select(&:marked_for_destruction?).each(&:destroy)
      end
    end
  end

end

And to make addresses deletable collection:

deletable_collection :addresses

The update! method is overwritten to skip elements with _destroy flag that are not persisted (without id) from being populated (mainly to work neatly with nested_form gem https://github.com/ryanb/nested_form)

The save! is ugly (respond_to?, ugh) but does the job: it destroys elements marked for destruction for deletable collections and works only with 2 levels of nesting so far (haven't ever needed more levels but it should be recursive to work in all cases).

Instead of making additional methods like deletable_collection and whitelisting collections, the best interface would be an additional option in collection method:

collection :addresses, deletable: true

What do you think? nested_form gem is quite commonly used and i think many developers could benefit from such feature working out of the box.

I'm not sure if the usecase is clear enough so here's a very simple example app: https://github.com/Azdaroth/reform_nested_delete_app

apotonick commented 9 years ago

Cool, thanks! I decided that this will go into disposable as there will be many more requests for application semantics in Reform (delete item, update partially, replace item, and so on). This shouldn't be implemented in a form object but in a cleanly separated layer (so we can use the same behaviour in Roar and representable).

Thanks for that, I now understand the requirements! :grimacing: I will keep you in the loop about the progress in disposable and will need your help integrating it with Reform.

Azdaroth commented 9 years ago

Great, it's going to be a damn useful feature :metal:

apotonick commented 9 years ago

It would be interesting to have a public discussion about what features we want to cover in this layer:

Azdaroth commented 9 years ago

Maybe touch option (to expire cache)? When deleting item I think it should be customizable, it would be possible then to destroy item, delete item, soft delete and whatever else you need.

apotonick commented 9 years ago

That will be handled with a similar technique to :populator where you can have strategies. Good idea.

I am working on another awesome feature (usable in disposable, cells, reform and roar): You can inject arbitrary additional objects into a form/cell/representer/twin via the constructor.

E.g. in a form this goes as follows.

SongRequestForm.new(song, requester: current_user)

The requester is then usable in the form as if it was a "real" model option. So helpful!

Azdaroth commented 9 years ago

Ha, great, no more overriding constructors :beers:

zavan commented 9 years ago

I'm also very interested in this, since I use cocoon for dynamic nested collections (https://github.com/nathanvda/cocoon), let me know if I can help with something.

artemave commented 9 years ago

Up until hitting this I was like "omg this reform thing is SO amazing". But now it feels a tinsy winsy bit incomplete... but still amazing!

apotonick commented 9 years ago

@artemave No no no!!!! tries to make angry face The fact that we carefully introduce model-related logic (such as deleting records) into a good object design within Reform shows that we actually care.

In Rails, this was all just pressed into accepts_nested_attributes without thinking about what other functionality might be needed and look at the mess that has created. Updating and delete works fine, but as soon as you need slightly more semantics it blows up!

I'm sorry that I haven't worked on this particular function a long time now, but trust me, the result will be so much better than this kludge you know from Rails. You will basically be able to specify operations what to do with each record via the form/JSON document.

artemave commented 9 years ago

@apotonick I am sure you're going to do the right thing.

And just to make up for the my rather pointless comment, here is a workaround I ended up with:

  # assuming our model has_many images
  collection :images, populate_if_empty: Image do
    property :id, writeable: false
    property :url
    property :filename
    property :_destroy, writeable: false
  end

  def save
    # you might want to wrap all this in a transaction
    super do |attrs|
      if model.persisted?
        to_be_removed = ->(i) { i[:_destroy] == '1' }
        image_ids_to_rm = attrs[:images].select(&to_be_removed).map {|i| i[:id] }

        Image.destroy(image_ids_to_rm)
        images.reject! {|i| image_ids_to_rm.include? i.id }
      end
    end

    # this time actually save
    super
  end

This is not a generic solution. But it got me through and, perhaps, someone will find it useful before the happy days arrive.

apotonick commented 9 years ago

That is very very generous and nice of you, @artemave ! You clearly understand the spirit of Open Source! :heart:

vala commented 9 years ago

For the time being, I managed as @artemave to find a temporary solution by creating a module :

module Reform
  module NestedForm
    extend ActiveSupport::Concern

    included do
      property :id
      property :_destroy
    end

    def _destroy=(value)
      model.mark_for_destruction if value == '1'
    end

    def sync_hash(options)
      options.exclude!([:id, :_destroy])
      super(options)
    end
  end
end

that I include in every Form I want to support Rails nested forms, this way

class OrderForm < Reform::Form
  collection :items do
    include Reform::Form

    property :price
  end
end

Hope this helps others some time while you're still working on a proper solution.

Thanks !

bkildow commented 9 years ago

Thank you @vala! I expanded on your example for a project which I needed to reject saving a nested model if a field was blank, as well as support for the cocoon gem. I'm somewhat new at ruby, but hopefully this work around helps someone as well: https://gist.github.com/bkildow/0c1bd0db8d1e4acdbb1c.

Also, thank you @apotonick . This gem is really great!

fernandes commented 9 years ago

hey guys... @bkildow thank you for you gist, helped me adding support on ActiveTrail for this feature while we don't have on Reform

@apotonick if you could please help me here: https://github.com/fernandes/activetrail/blob/e84af94c43495a07d0ca0b0337bc4c5c768fefc6/spec/internal/app/models/thing.rb#L33 why its trying to change a frozen hash... I got into ActiveRecord::AutosaveAssociation but got nothing related to this

apotonick commented 9 years ago

This feature should be available in Reform in the next week - I almost finished preparing work in Disposable and will then make it available in Reform (adding, skipping, replacing, deleting items). :beers:

Azdaroth commented 9 years ago

@apotonick that's awesome! Really been waiting for that feature :beer: :beers: .

artemave commented 9 years ago

@apotonick for president!

fernandes commented 9 years ago

@apotonick prost! :beers:

apotonick commented 9 years ago

Chapter 7 shows how to delete manually: https://github.com/apotonick/gemgem-trbrb/blob/chapter-7/app/concepts/thing/crud.rb#L81

However, in chapter 9 or later I will demonstrate how to use "Collection Semantics" from Representable (new upcoming feature).

fernandes commented 9 years ago

great!! :beers:

christian-froh commented 9 years ago

I tried to implement cocoon in the newest reform, but i get this error: undefined method `reflect_on_association' for MuseumForm:Class

Do I miss something?

apotonick commented 9 years ago

include Reform::Form::ActiveModel::ModelReflections. How on earth is that related to this thread?

christian-froh commented 9 years ago

cause cocoon was mentioned here couple of times ;)

lucaspiller commented 8 years ago

I know this thread has been dead for over a year, but I came across it trying to get nested forms + Cocoon working. The API for Reform 2.2 has changed quite a bit, so here is an example (based on @bkildow's) that works with Reform 2.2.1:

https://gist.github.com/lucaspiller/615f09bb525a14163921fd56b4b8e611