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

Recursively process nested parameters for polymorphism #1648

Merged
merged 1 commit into from
May 29, 2020
Merged

Recursively process nested parameters for polymorphism #1648

merged 1 commit into from
May 29, 2020

Conversation

efatsi
Copy link
Contributor

@efatsi efatsi commented May 14, 2020

Not sure what the best way to test this would be. I could manually build a parameter hash to simulate that which comes in from NestedHasMany? Or perhaps this is straightforward enough to not require specific testing?

@nickcharlton
Copy link
Member

It'd be good to have a test here. I'd suggesting going ahead with a manually built parameter hash — I can't think of a better plan.

@nickcharlton nickcharlton added bug breakages in functionality that is implemented dashboards how administrate presents fields and displays data models models, associations and fetching the underlying data labels May 22, 2020
@efatsi
Copy link
Contributor Author

efatsi commented May 27, 2020

@nickcharlton added a spec here, verified that it failed prior to the change in ApplicationController and passes now. Let me know what you think.

Copy link
Member

@nickcharlton nickcharlton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just one comment on the exception we're deliberately suppressing.

The CI failures are because of now updated packages, so if you rebase these will go away. If you could look at the Hound comments too, that'd be great!

post :create, log_entry: resource_params
rescue ActiveModel::UnknownAttributeError
# Ignore errors that arise because the parameters are contrived to
# test that nested polymorphic parameters are properly converted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this happen with this test? I wonder if there's a way around it if so

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does happen, because the params that are being used to attempt to create a LogEntry record are fictitious and don't align with viable attributes. I'll try tossing spinning up an attr_accessor on the model, my first thought was to mock in the entire NestedHasMany logic to make those params valid but now that you ask I think there's probably a quick workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickcharlton attr_accessor worked great, pushed a commit up for that + hound fixes. Only hound stuff left looks ignorable (I think).


LogEntry.attr_accessor :arbitrarily

post :create, log_entry: resource_params
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails/HttpPositionalArguments: Use keyword arguments instead of positional arguments for http call: post.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to ignore this one.

When submitting changes to models with polymorphic nested parameters, we
previously weren't handling all of the parameters through the data
structure given to us. This ensures we do that by doing it recursively.

Fixes nickcharlton/administrate-field-nested_has_many#38
@nickcharlton nickcharlton merged commit 8bff96f into thoughtbot:master May 29, 2020
@nickcharlton
Copy link
Member

Thanks for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug breakages in functionality that is implemented dashboards how administrate presents fields and displays data models models, associations and fetching the underlying data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants