Errors can be indexed with nested attributes #19686

Merged
merged 1 commit into from Oct 26, 2015

Conversation

Projects
None yet
@mprobber
Contributor

mprobber commented Apr 7, 2015

accepts_nested_attributes_for can now take index_errors: true as an
option. When this is enabled, errors for nested models will be
returned alongside an index, as opposed to just the nested model name.
This option can also be enabled globally in a configuration file.

@tsun1215
rails/rails#8638

activerecord/test/cases/helper.rb
@@ -21,6 +21,9 @@
# Show backtraces for deprecated behavior for quicker cleanup.
ActiveSupport::Deprecation.debug = true
+# Don't add indeces to nested attribute errors

This comment has been minimized.

@tsun1215

tsun1215 Apr 7, 2015

Contributor

Will change, but isn't it indices?

@tsun1215

tsun1215 Apr 7, 2015

Contributor

Will change, but isn't it indices?

activerecord/CHANGELOG.md
+ now be indexed if :index_errors is specified when defining the nested
+ attributes, or if its set in the global config.
+
+ *Michael Probber and Terence Sun*

This comment has been minimized.

@mprobber

mprobber Apr 7, 2015

Contributor

done!

@mprobber

mprobber Apr 7, 2015

Contributor

done!

return true if record.destroyed? || record.marked_for_destruction?
validation_context = self.validation_context unless [:create, :update].include?(self.validation_context)
unless valid = record.valid?(validation_context)
if reflection.options[:autosave]
record.errors.each do |attribute, message|
- attribute = "#{reflection.name}.#{attribute}"
+ options = self.nested_attributes_options[reflection.name]

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

I think we can move this out of the loop, it will not change.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

I think we can move this out of the loop, it will not change.

+ elsif index.nil? or options.nil? or not options[:index_errors]
+ attribute = "#{reflection.name}.#{attribute}"
+ else
+ attribute = "#{reflection.name}[#{index}].#{attribute}"

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

This branch is the same as the first, so I think we can improve this conditionals to have only two branches.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

This branch is the same as the first, so I think we can improve this conditionals to have only two branches.

This comment has been minimized.

@tsun1215

tsun1215 Apr 8, 2015

Contributor

We originally had it as 2 branches, but the first branch looked excessively long. Should we still do 2 branches?

@tsun1215

tsun1215 Apr 8, 2015

Contributor

We originally had it as 2 branches, but the first branch looked excessively long. Should we still do 2 branches?

@@ -304,9 +304,9 @@ module ClassMethods
# # creates avatar_attributes= and posts_attributes=
# accepts_nested_attributes_for :avatar, :posts, allow_destroy: true
def accepts_nested_attributes_for(*attr_names)
- options = { :allow_destroy => false, :update_only => false }
+ options = { :allow_destroy => false, :update_only => false, :index_errors => false }

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

Use new hash syntax

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

Use new hash syntax

This comment has been minimized.

@eileencodes

eileencodes Apr 8, 2015

Member

@mprobber - I'm not sure if we went over that at open academy, using the new hash syntax means it should read like this instead:

options = { allow_destroy: false, update_only: false, index_errors: false }

@eileencodes

eileencodes Apr 8, 2015

Member

@mprobber - I'm not sure if we went over that at open academy, using the new hash syntax means it should read like this instead:

options = { allow_destroy: false, update_only: false, index_errors: false }

options.update(attr_names.extract_options!)
- options.assert_valid_keys(:allow_destroy, :reject_if, :limit, :update_only)
+ options.assert_valid_keys(:allow_destroy, :reject_if, :limit, :update_only, :index_errors)

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

I think we can use kwargs here and avoif the extract_options!, the assert_valid_keys and the default options altogether.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

I think we can use kwargs here and avoif the extract_options!, the assert_valid_keys and the default options altogether.

activerecord/test/cases/helper.rb
@@ -21,6 +21,9 @@
# Show backtraces for deprecated behavior for quicker cleanup.
ActiveSupport::Deprecation.debug = true
+# Don't add indices to nested attribute errors
+ActiveRecord::Base.index_nested_attribute_errors = false

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

This should be the default value already, so we should not need to set it on the test.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

This should be the default value already, so we should not need to set it on the test.

@@ -32,5 +32,6 @@ class Application < Rails::Application
# The default locale is :en and all translations from config/locales/*.rb,yml are auto loaded.
# config.i18n.load_path += Dir[Rails.root.join('my', 'locales', '*.{rb,yml}').to_s]
# config.i18n.default_locale = :de
+ config.index_nested_attributes_errors = true

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

This doesn't work. We don't have any global config with this name. We should set it inside the active_record namespace. Also it should not be inside the config/application and I don't even think it should be true by default. Let just document it on the configuring guide and not include in the generated application.

@rafaelfranca

rafaelfranca Apr 8, 2015

Member

This doesn't work. We don't have any global config with this name. We should set it inside the active_record namespace. Also it should not be inside the config/application and I don't even think it should be true by default. Let just document it on the configuring guide and not include in the generated application.

+ options = self.nested_attributes_options[reflection.name]
+ if ActiveRecord::Base.index_nested_attribute_errors
+ attribute = "#{reflection.name}[#{index}].#{attribute}"
+ elsif index.nil? or options.nil? or not options[:index_errors]

This comment has been minimized.

@kaspth

kaspth Apr 8, 2015

Member

Let's use || and ! here to be in line with our style guide 😄

@kaspth

kaspth Apr 8, 2015

Member

Let's use || and ! here to be in line with our style guide 😄

This comment has been minimized.

@tsun1215

tsun1215 Apr 8, 2015

Contributor

Done. We'll update once we figured out how we should break the conditional up (see above).

@tsun1215

tsun1215 Apr 8, 2015

Contributor

Done. We'll update once we figured out how we should break the conditional up (see above).

Errors can be indexed with nested attributes
`has_many` can now take `index_errors: true` as an
option.  When this is enabled, errors for nested models will be
returned alongside an index, as opposed to just the nested model name.
This option can also be enabled (or disabled) globally through
`ActiveRecord::Base.index_nested_attribute_errors`

E.X.

```ruby
class Guitar < ActiveRecord::Base
  has_many :tuning_pegs
  accepts_nested_attributes_for :tuning_pegs
end

class TuningPeg < ActiveRecord::Base
  belongs_to :guitar
  validates_numericality_of :pitch
end
```

 - Old style
 - `guitar.errors["tuning_pegs.pitch"] = ["is not a number"]`

 - New style (if defined globally, or set in has_many_relationship)
 - `guitar.errors["tuning_pegs[1].pitch"] = ["is not a number"]`

[Michael Probber, Terence Sun]

@sgrif sgrif merged commit 21e448b into rails:master Oct 26, 2015

sgrif added a commit that referenced this pull request Oct 26, 2015

Merge pull request #19686 from tsun1215/index_errors
Errors can be indexed with nested attributes

Close #8638
+
+ For models which have nested attributes, errors within those models will
+ now be indexed if :index_errors is specified when defining a
+ has_many relationship, or if its set in the global config.

This comment has been minimized.

@s-aida

s-aida Oct 26, 2015

Contributor

typo of it's?

@s-aida

s-aida Oct 26, 2015

Contributor

typo of it's?

This comment has been minimized.

@sgrif

sgrif Oct 26, 2015

Member

No, that is the correct way to spell its here

On Mon, Oct 26, 2015, 5:20 PM Shunsuke Aida notifications@github.com
wrote:

In activerecord/CHANGELOG.md
#19686 (comment):

@@ -1,3 +1,31 @@
+* Add option to index errors in nested attributes
+

  • For models which have nested attributes, errors within those models will
  • now be indexed if :index_errors is specified when defining a
  • has_many relationship, or if its set in the global config.

typo of it's?


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/19686/files#r43067124.

@sgrif

sgrif Oct 26, 2015

Member

No, that is the correct way to spell its here

On Mon, Oct 26, 2015, 5:20 PM Shunsuke Aida notifications@github.com
wrote:

In activerecord/CHANGELOG.md
#19686 (comment):

@@ -1,3 +1,31 @@
+* Add option to index errors in nested attributes
+

  • For models which have nested attributes, errors within those models will
  • now be indexed if :index_errors is specified when defining a
  • has_many relationship, or if its set in the global config.

typo of it's?


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/19686/files#r43067124.

This comment has been minimized.

@jeremy

jeremy Oct 26, 2015

Member

It's best you check its meaning 🤓

@jeremy

jeremy Oct 26, 2015

Member

It's best you check its meaning 🤓

This comment has been minimized.

@jeremy

jeremy Oct 26, 2015

Member

Consider a user reading this. Where would you find "global config?"

@jeremy

jeremy Oct 26, 2015

Member

Consider a user reading this. Where would you find "global config?"

This comment has been minimized.

@jonatack

jonatack Oct 31, 2015

Contributor

its indicates possession.
it's is a contraction for it is.
The latter seems to be the case here. If so, it is would be even better.

@jonatack

jonatack Oct 31, 2015

Contributor

its indicates possession.
it's is a contraction for it is.
The latter seems to be the case here. If so, it is would be even better.

+ - `guitar.errors["tuning_pegs.pitch"] = ["is not a number"]`
+
+ - New style (if defined globally, or set in has_many_relationship)
+ - `guitar.errors["tuning_pegs[1].pitch"] = ["is not a number"]`

This comment has been minimized.

@jeremy

jeremy Oct 26, 2015

Member

This example doesn't show either way to configure indexed errors. This is the place to spell it out.

@jeremy

jeremy Oct 26, 2015

Member

This example doesn't show either way to configure indexed errors. This is the place to spell it out.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Oct 27, 2015

Contributor

Excellent stuff guys!

Contributor

bogdan commented Oct 27, 2015

Excellent stuff guys!

@rweng

This comment has been minimized.

Show comment
Hide comment
@rweng

rweng Oct 31, 2015

pulled it in on top of 4.2.4 and it works like a charm. Would love to see this change being in 4.2.5 so I can discard my fork :)

rweng commented Oct 31, 2015

pulled it in on top of 4.2.4 and it works like a charm. Would love to see this change being in 4.2.5 so I can discard my fork :)

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Oct 31, 2015

Member

Sorry, we don't backport features to patch releases. This'll be in Rails 5.

Member

kaspth commented Oct 31, 2015

Sorry, we don't backport features to patch releases. This'll be in Rails 5.

@uberllama

This comment has been minimized.

Show comment
Hide comment
@uberllama

uberllama Nov 19, 2015

Contributor

Long overdue. Thanks for your effort. 🏁

Contributor

uberllama commented Nov 19, 2015

Long overdue. Thanks for your effort. 🏁

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 9, 2016

mtomov added a commit to mtomov/solidus that referenced this pull request Sep 14, 2016

Improve access to order from populate action
  * Using an instance variable allows JS populate operations to have
    access to the @order from within the rendered .js.erb files

  * Assign all line_item errors to the order's base

  * Allow to respond to JS requests for `#populate` and `#update` actions.
    It is left for the developer to actually implement the `js.erb` views


To consider / TODO:

  - to avoid the whole rescue operation, and manually assigning line item
    errors to the order, we'd need to do to be doing `order.save`
    instead of `line_item.save!` in [`OrderContents`](https://github.com/solidusio/solidus/blob/master/core/app/models/spree/order_contents.rb#L151)

  -> That would have the effect to "automatically" assign the line item
     errors on the order in the form: `line_items.quantity` in Rails 4.x
     and "line_items[0].quantity" in Rails 5


  More info:
  - rails/rails#8638
  - rails/rails#19686

@mtomov mtomov referenced this pull request in solidusio/solidus Sep 14, 2016

Merged

Improve JS handling for OrdersController #1440

mtomov added a commit to mtomov/solidus that referenced this pull request Sep 15, 2016

Improve access to order from populate action
  * Using an instance variable allows JS populate operations to have
    access to the @order from within the rendered .js.erb files

  * Assign all line_item errors to the order's base

  * Allow to respond to JS requests for `#populate` and `#update` actions.
    It is left for the developer to actually implement the `js.erb` views


To consider / TODO:

  - to avoid the whole rescue operation, and manually assigning line item
    errors to the order, we'd need to do to be doing `order.save`
    instead of `line_item.save!` in [`OrderContents`](https://github.com/solidusio/solidus/blob/master/core/app/models/spree/order_contents.rb#L151)

  -> That would have the effect to "automatically" assign the line item
     errors on the order in the form: `line_items.quantity` in Rails 4.x
     and "line_items[0].quantity" in Rails 5


  More info:
  - rails/rails#8638
  - rails/rails#19686

mtomov added a commit to mtomov/solidus that referenced this pull request Sep 18, 2016

Improve access to order from populate action
  * Using an instance variable allows JS populate operations to have
    access to the @order from within the rendered .js.erb files

  * Assign all line_item errors to the order's base

  * Allow to respond to JS requests for `#populate` and `#update` actions.
    It is left for the developer to actually implement the `js.erb` views


To consider / TODO:

  - to avoid the whole rescue operation, and manually assigning line item
    errors to the order, we'd need to do to be doing `order.save`
    instead of `line_item.save!` in [`OrderContents`](https://github.com/solidusio/solidus/blob/master/core/app/models/spree/order_contents.rb#L151)

  -> That would have the effect to "automatically" assign the line item
     errors on the order in the form: `line_items.quantity` in Rails 4.x
     and "line_items[0].quantity" in Rails 5


  More info:
  - rails/rails#8638
  - rails/rails#19686

mtomov added a commit to mtomov/solidus that referenced this pull request Sep 20, 2016

Improve respond to JS from `OrdersController#populate` action
  * Using an instance variable allows JS populate operations to have
    access to the @order from within the rendered .js.erb files

  * Assign all line_item errors to the order's base

  * Allow to respond to JS requests for `#populate` and `#update` actions.
    It is left for the developer to actually implement the `js.erb` views

To consider / TODO:

  - to avoid the whole rescue operation, and manually assigning line item
    errors to the order, we'd need to do to be doing `order.save`
    instead of `line_item.save!` in [`OrderContents`](https://github.com/solidusio/solidus/blob/master/core/app/models/spree/order_contents.rb#L151)

  -> That would have the effect to "automatically" assign the line item
     errors on the order in the form: `line_items.quantity` in Rails 4.x
     and "line_items[0].quantity" in Rails 5

  More info:
  - rails/rails#8638
  - rails/rails#19686

bbuchalter added a commit to TommyJohnWear/solidus that referenced this pull request Nov 18, 2016

Improve respond to JS from `OrdersController#populate` action
  * Using an instance variable allows JS populate operations to have
    access to the @order from within the rendered .js.erb files

  * Assign all line_item errors to the order's base

  * Allow to respond to JS requests for `#populate` and `#update` actions.
    It is left for the developer to actually implement the `js.erb` views

To consider / TODO:

  - to avoid the whole rescue operation, and manually assigning line item
    errors to the order, we'd need to do to be doing `order.save`
    instead of `line_item.save!` in [`OrderContents`](https://github.com/solidusio/solidus/blob/master/core/app/models/spree/order_contents.rb#L151)

  -> That would have the effect to "automatically" assign the line item
     errors on the order in the form: `line_items.quantity` in Rails 4.x
     and "line_items[0].quantity" in Rails 5

  More info:
  - rails/rails#8638
  - rails/rails#19686

cerdiogenes added a commit to cerdiogenes/active_type that referenced this pull request Feb 7, 2017

@cerdiogenes cerdiogenes referenced this pull request in makandra/active_type Feb 8, 2017

Merged

index_errors option to nests_many #74

@cerdiogenes

This comment has been minimized.

Show comment
Hide comment
@cerdiogenes

cerdiogenes Feb 8, 2017

I can't find a way to parse my errors messages:

errors = {
  "categories[0].listener_deadline_values[0].value": ["can't be blank"],
  "categories[0].listener_deadline_values[0].deadline": ["can't be blank"],
  "categories[0].name": ["can't be blank"],
  "categories[1].listener_deadline_values[0].value": ["can't be blank"],
  "categories[1].listener_deadline_values[0].deadline": ["can't be blank"],
  "categories[1].name": ["can't be blank"],
  "categories[2].listener_deadline_values[0].value": ["can't be blank"],
  "categories[2].listener_deadline_values[0].deadline": ["can't be blank"],
  "categories[2].name": ["can't be blank"]
}

I would expect to be able to access these errors like errors.categories[2].name

How do you parse it? I discovered how: https://medium.com/@kdiogenes/giving-super-powers-to-rails-nested-forms-with-vue-js-part-2-acee4a3ee43d

cerdiogenes commented Feb 8, 2017

I can't find a way to parse my errors messages:

errors = {
  "categories[0].listener_deadline_values[0].value": ["can't be blank"],
  "categories[0].listener_deadline_values[0].deadline": ["can't be blank"],
  "categories[0].name": ["can't be blank"],
  "categories[1].listener_deadline_values[0].value": ["can't be blank"],
  "categories[1].listener_deadline_values[0].deadline": ["can't be blank"],
  "categories[1].name": ["can't be blank"],
  "categories[2].listener_deadline_values[0].value": ["can't be blank"],
  "categories[2].listener_deadline_values[0].deadline": ["can't be blank"],
  "categories[2].name": ["can't be blank"]
}

I would expect to be able to access these errors like errors.categories[2].name

How do you parse it? I discovered how: https://medium.com/@kdiogenes/giving-super-powers-to-rails-nested-forms-with-vue-js-part-2-acee4a3ee43d

@tommyalvarez

This comment has been minimized.

Show comment
Hide comment
@tommyalvarez

tommyalvarez May 23, 2017

Is there a way to index by another criteria @mprobber ? Like the ID of the nested model if it's not a new record? This is because i can't figure a way when using rails 5 api to map errors into several nested children which some of them can have errors while others not, therefore indexes won't match one on one. Or is there another clever solution to handle this situation?

Is there a way to index by another criteria @mprobber ? Like the ID of the nested model if it's not a new record? This is because i can't figure a way when using rails 5 api to map errors into several nested children which some of them can have errors while others not, therefore indexes won't match one on one. Or is there another clever solution to handle this situation?

@vidurangaw

This comment has been minimized.

Show comment
Hide comment
@vidurangaw

vidurangaw Aug 28, 2017

@tommyalvarez
I'm having the exact issue you've faced. Were you able to find a solution?

@tommyalvarez
I'm having the exact issue you've faced. Were you able to find a solution?

@tommyalvarez

This comment has been minimized.

Show comment
Hide comment
@tommyalvarez

tommyalvarez Aug 28, 2017

@vidurangaw i ended up writing my own custom NestedErrorSerializer (i was using active model serializer with json-api adapter). This way i was able to return the errors the way i wanted, respeting json-api format. Thought it would take me a lot of time but it wasn't that hard at all. Here is the implementation i made:

module NestedErrorSerializer
  # object is the object to serialize, relationships an array of symbols with the relationships of the object
  def self.serialize(object, relationships)
    errors = object.errors.messages.map do |field, errors|
      errors.map do |error_message|
        {
          source: {pointer: "/data/attributes/#{field}"},
          detail: error_message
        }
      end
    end
    relationships.each do |relationship|
      object.send(relationship).each_with_index do |child, index|
        errors << child.errors.messages.map do |field, errors|
          errors.map do |error_message|
            {
              source: {pointer: "/data/attributes/#{child.model_name.plural}[#{index}].#{field}"},
              detail: error_message
            }
          end
        end
      end
    end if relationships.present?
    errors.flatten
  end
end

Then in the controller i used them like:

render json: {errors: NestedErrorSerializer.serialize(@product, [:consumables])}, status: :unprocessable_entity

@vidurangaw i ended up writing my own custom NestedErrorSerializer (i was using active model serializer with json-api adapter). This way i was able to return the errors the way i wanted, respeting json-api format. Thought it would take me a lot of time but it wasn't that hard at all. Here is the implementation i made:

module NestedErrorSerializer
  # object is the object to serialize, relationships an array of symbols with the relationships of the object
  def self.serialize(object, relationships)
    errors = object.errors.messages.map do |field, errors|
      errors.map do |error_message|
        {
          source: {pointer: "/data/attributes/#{field}"},
          detail: error_message
        }
      end
    end
    relationships.each do |relationship|
      object.send(relationship).each_with_index do |child, index|
        errors << child.errors.messages.map do |field, errors|
          errors.map do |error_message|
            {
              source: {pointer: "/data/attributes/#{child.model_name.plural}[#{index}].#{field}"},
              detail: error_message
            }
          end
        end
      end
    end if relationships.present?
    errors.flatten
  end
end

Then in the controller i used them like:

render json: {errors: NestedErrorSerializer.serialize(@product, [:consumables])}, status: :unprocessable_entity

@vidurangaw

This comment has been minimized.

Show comment
Hide comment
@vidurangaw

vidurangaw Aug 28, 2017

@tommyalvarez
Thanks for the quick response. I'll try this

@tommyalvarez
Thanks for the quick response. I'll try this

@aruprakshit

This comment has been minimized.

Show comment
Hide comment
@aruprakshit

aruprakshit Sep 10, 2017

Why this option is not mentioned in the official doc ?

Why this option is not mentioned in the official doc ?

@evanrolfe evanrolfe referenced this pull request in openSUSE/open-build-service Oct 16, 2017

Merged

Kiwi Editor - Add understandable flash message #3981

@dhyegofernando

This comment has been minimized.

Show comment
Hide comment
@dhyegofernando

dhyegofernando Jan 26, 2018

I've created a gem (active_record-nested_error_indexer) to monkey patch these changes to Rails 4 😄

I've created a gem (active_record-nested_error_indexer) to monkey patch these changes to Rails 4 😄

@brendanthomas1 brendanthomas1 referenced this pull request in thoughtbot/shoulda-matchers Apr 4, 2018

Closed

Add index_errors association matcher #1089

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment