Fix option_values nested attributes behavior on the API #4409
Conversation
As the Rails documentation makes clear [1], nested attributes must be explicitly defined. Otherwise, they default to an empty Hash. [1] - https://api.rubyonrails.org/classes/ActionController/StrongParameters.html
There was a problem hiding this comment.
Thanks, Marc. If I got it correctly, all existing stores won't be affected because we are adding more attributes to the permitted list and not removing any of the existing ones.
But, if someone was using option_values_attributes on the variant endpoints, would their requests break? Also, please take a look at the API docs here. If I recall correctly that variant_attributes is also used to define the API available params.
There's not a nested attributes definition for option values in variants. On top of that, having `:option_values_attributes` declared as a plain Symbol would have no effect. Rails requires that nested attributes are explicitly declared [1]. Variants already support `:option_value_ids` array [2] which cover the most common use case (adding option values to an already created variant), so there's no need to provide such a nested hierarchy. 1 - https://api.rubyonrails.org/classes/ActionController/StrongParameters.html 2 - https://github.com/solidusio/solidus/blob/07c88ddd1603699939ac0343965fa88dc2e1851a/core/lib/spree/permitted_attributes.rb#L135
waiting-for-dev
force-pushed
the
waiting-for-dev/variant_option_values_api
branch
from
3b30854 to
c14578b
Compare
No. The attribute will be ignored (not permitted). The difference is that both keys and values will now be ignored, while until now, the given value reached the model layer but was ignored once there. Notice that there's no definition of nested option values within variants: irb(main):005:0> Spree::Variant.new.option_values_attributes=[]
/home/solidus_user/gems/gems/activemodel-7.0.3/lib/active_model/attribute_methods.rb:458:in `method_missing': undefined method `option_values_attributes=' for #<Spree::Variant id: nil, sku: "", weight: 0.0, height: nil, width: nil, depth: nil, deleted_at: nil, is_master: false, product_id: nil, cost_price: nil, position: nil, cost_currency: nil, track_inventory: true, tax_category_id: nil, updated_at: nil, created_at: nil> (NoMethodError)
Did you mean? option_values_variants=
option_values_variants
I'm not sure I follow. I removed it from the docs right now. Is that what you meant? |
Ah, now it's clear, thanks.
Yes, exactly. Thanks Marc! |
Description
Both for option types and variants,
:option_values_attributeswas defined as a plain Symbol. As the Rails documentation makes clear, nested attributes must be explicitly defined. Otherwise, they default to an empty Hash. On top of that, there's not a nested attributes definition for option values in variants.We fix it for option types and remove it altogether for variants. Variants already support
:option_value_idsarray, which covers the most common use case (adding option values to an already created variant), so there's no need to provide such a nested hierarchy.Checklist: