Fixed functionality to include method in params_wrapper.rb

[ryanrperez]

to properly wrap all attributes, including those which are nested.

fix for issue #17216

@Sxl1092

These param_wrapper methods are not tested in ActiveRecord. We looked through ActiveRecord and found some nested_attributes tests though that runs tests on all of the nested relations. The problem is that these just tests the nested relationships directly, and we need a test that makes sure that our method in param_wrappers returns all of these nested attributes. We are still stuck on how to get a test for this controller method. We cannot do it in ActiveRecord because those don't test controllers, and in ActionPack these controller tests do not interact with any models. We tried looking at method stubs and we are wondering if that might be a potential solution? We are confused about how to run a simple controller test though if we can't get our controller to interact with some model(s). If there are some basic examples in the current tests that do this, or perhaps some general best coding practice that we are not aware of, we would appreciate any advice.

Our general approach is correct we believe, because we're trying to access both the attribute_names and nested_attributes_options to make sure that our changes correctly return some combination of these, but we're not sure how to grab these attributes and nested_attributes if we cannot access a model from the controller. And if we were in ActiveRecord, we wouldn't know how to use our controller method on the attributes that we have in those files. This crossover between the two might not be necessary at all, but if that's the case we are not sure how we should be trying to test it.

[matthewd]

Is there an open issue this is fixing?

[ryanrperez]

Yes, edited above description to reference issue #17216

[Quintasan]

It absolutely baffles me that this has not been fixed and merged yet. It's my third time running into this when building JSON API and that's the third time I'm wondering if there's any intention to fix this at all.

[ammarshah]

Came across the same issue :(

[lypanov]

IMHO if this PR won't be merged the wrapping feature should just be deleted from Rails.

[betesh]

👍

[ghost]

👍 bump

[terry90]

Will it merged ?

[kmanzana]

Is this not being merged because there is a failure in the CI? Just one test failing

[rafaelfranca]

Yes, it also needs a test for the problem it is fixing. Could someone take a look on working in a test?

[Quintasan]

I guess I'll take a stab at it this weekend. Don't expect wonders though.

[rmcsharry]

I just stumbled on this problem today with an Angular4, Rails 5.1.4 app.

Sadly I just lost 8 hours of my life trying to figure out why the heck my nested attributes were disappearing! And several hours of others on stack overflow looking at it too.

I never EVER would have guessed 8 hours ago that wrap_parameters was causing this behaviour. So if wrap_parameters did not exist, I would have used interceptors in Angular to wrap nodes.

If it's not possible to write a test due to the Catch-22 model/controller issue I honestly think wrap_parameters feature should be removed from Rails. Angular 1 has interceptors and Angular 4.3 got them 10 months ago. Ember has had them since 2.something. I'd imagine in 2017 most js frameworks do.

So...I wish @Quintasan the best of luck in writing a test! But if not successful, perhaps consider letting the axe fall on this 'feature'?

[kmanzana]

PR to this PR open at ryanrperez#1. So much history that I had to pull, just look at the last commit on that PR. Might just open up a new PR since the code was broken anyway, but we'll see if @ryanrperez can take care of it.

[rafaelfranca]

@kmanzana maybe it is better if you cherry-pick the commit from this PR in a new, updated banch, apply your commmit on top of it and open a new PR against Rails from your branch.

[kmanzana]

@rafaelfranca, good call. Opened #30965.

[kamipo]

Merged at 62b7ad4.