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

Show error for collection on correct index #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fernandes
Copy link
Member

@fernandes fernandes commented Mar 12, 2018

UPDATE: rewrote the description once this is a feature proposal, not a fix

@doits said on gitter that reform is missing a rails 5 validation feature, index nested attribute errors (which is false by default)

When there's a collection like:

    collection :tracks, populate_if_empty: Track do
      property :title

      validates :title, presence: true
    end

and in rails is with option enabled, instead of returning a "global" error for all elements (reform default behaviour) is like this

{"tracks_attributes.title":[{"error":"can't be blank"}]}

but when option is enabled, it should return something like:

{"tracks_attributes[1].title":[{"error":"can't be blank"}]}

so it's possible to know what is the invalid element

@doits
Copy link

doits commented Mar 12, 2018

I think for Rails to return indexed errors you need to enable it either globally ...

Rails.application.config.active_record.index_nested_attribute_errors = true

... or directly in the model's association ...

class AlbumTracks < ActiveRecord::Base
  has_many :tracks, index_errors: true
end

Might be a good idea to implement it in a similar fashion here, maybe having index_errors on the collection method ...

class AlbumTracksForm < Reform::Form
  collection :tracks, index_errors: true do
    ...
  end
end

... is enough (though adding a global config does not hurt, either).

@fernandes
Copy link
Member Author

@doits thank you for further information... and checking some reform test the current is exactly this not-indexed error

doing a better research it's a feature added in rails 5, so this PR is more a discussion for a new feature, instead of a bug fix, I'm gonna update the description

@apotonick
Copy link
Member

We kind of have this feature out of the box, and much cleaner than Rails 5: If you want to know a specific item's errors, you can do form.items[2].errors, which is not possible in Rails, so even though we can add those counter-intuitive error message keys, they're a work-around from Rails that I'm not so sure we should encourage.

@doits
Copy link

doits commented Mar 13, 2018

I too think that the Rails' syntax is awkward, but maybe it could be still added to be consistent with Rails.

Anyway, for me it would be enough to get the errors sanely and correctly indexed, maybe like this:

errors: {
  title: { error: "can't be blank" } # <-- title of base model is blank
  items: [ # <-- items is a collection
    {}, # <-- first item has no errors
    { title: { error: "can't be blank" } } # <-- second item has some error
  ]
}

Is this currently possible, maybe by looping over some structure manually? Is there a way to loop over every property and collection and fetch the errors?

@apotonick
Copy link
Member

@doits I am pretty sure the current implementation in Reform 2.3.0.rc2 does that, you can say form.items.errors and it compiles you a list?!

@fran-worley
Copy link
Contributor

fran-worley commented Mar 13, 2018 via email

@doits
Copy link

doits commented Mar 13, 2018

I'm at 2.2.4 currently, will just try out 2.3.0.rc1 (which seems to be the latest version at rubygems) and report back.

@doits
Copy link

doits commented Mar 13, 2018

@apotonick I just checked out master from github to have the latest verisions of reform and reform-rails.

I have this form:

  property :first_name
  validates :first_name, presence: true

  collection :addresses, populate_if_empty: Address do
    property :city

    validates :city, presence: true
  end

I cannot do form.addresses.errors --> undefined method 'errors' for #<Disposable::Twin::Collection>.

I came up with the following though:

  def all_errors
    base = errors.messages
    base[:addresses] = addresses.map do |address|
      address.errors.messages
    end
    base
  end

So form.all_errors gives me:

{
  :first_name=>["muss ausgefüllt werden"],
  :"addresses.city"=>["muss ausgefüllt werden"],
  :addresses=>[
    {}, {:city=>["muss ausgefüllt werden"]}
  ]
}

The addresses.city key is too much (but can be simply ignored or could be filtered out).

But is there any way I can loop over all collection? Currently I must know that addresses is a collection. I'd rather do it generally, so I can use it for all forms, like this:

  def all_errors
    base = errors.messages

    # base[:addresses] = addresses.map do |address|
    #   address.errors.messages
    # end

    # is there something like `all_collections` to do this?
    all_collections.each do |collection_name|
      base[collection_name] = send(collection_name).map do |item|
        item.errors.messages
      end
    end

    base
  end

@fernandes
Copy link
Member Author

@doits yeah, your all_errors is a great way to check... in the failing test, I can check:

nested_form.tracks[0].errors.must_equal {title: ['can\'t be blank']}

works great

@fran-worley you mean that form.items.errors should compile a list?

@apotonick @fran-worley thanks for quick reply! ☺️

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 this pull request may close these issues.

4 participants