Automatic associations

[pablobm]

Fixes #1597

Summary

Currently, associative dashboard fields (BelongsTo, HasMany, HasOne) require explicit configuration options (:class_name, :foreign_key, :primary_key) for Administrate to find their associated models. There are details that Administrate can figure out automatically, but not everything, and not always reliably.

This PR changes these field types so that they do not need this configuration, instead detecting associations by using ActiveRecord's reflection API.

Implementation notes

The core change of the feature specs is at spec/example_app/app/dashboards/customer_dashboard.rb. I started by introducing that change, then proceeded slowly to make everything work.

This implementation relies on Field::Base.new always receiving a :resource_class option. I think this is safe as it's already the case throughout the codebase. However third party gems may have incompatibilities here.

Third party gems

I have had a look at third-party plugins listed at https://rubygems.org/gems/administrate/reverse_dependencies. From this list, only administrate-field-nested_has_many appear to have incompatibilities, which is not surprising as it adds a new associative field with some advanced features. Specifically you'll find that:

• It will not work if you remove your :class_name options from NestedHasMany fields.
• You can work around this by continuing to use the deprecated options (just for your NestedHasMany fields), although you'll get deprecation warnings.
• I created a branch that will bring this plugin in sync with the changes in this PR: nickcharlton/administrate-field-nested_has_many@master...pablobm:automatic-associations

Deprecations

I could have removed the deprecated options completely, but I decided against it because:

• I want to minimise disruption when updating Administrate.
• I don't have 100% trust that just removing the options will work for everyone. There may be a corner case I'm missing.

Instead I'm allowing the old behaviour to exist for a bit longer, while showing abundant deprecation warnings. Hopefully this will get people to upgrade quickly and report any incompatibility.

General review tips

The option "Hide whitespace changes" is very useful here, as I have changed a lot of indent at the specs, in order to avoid style warnings (Hound).

Also due to style warnings, I have reformatted a lot of code, adding more diff noise in the process.

So this is to say: this MR looks huge, but it's just... large, I guess.

The future

The current code relies a lot on globals (eg: class methods) that are difficult to test and generally work with. In the future, we may want to change the interface for field types to improve the situation, but I think that would be too much for this PR.

There's still some risky string-to-class metaprogramming going on, for example a "#{associated_class_name}Dashboard".constantize.new in lib/administrate/field/associative.rb (not shown in the diff as unchanged). I have let this be for now. I think it should be reviewed in the future as part of work in the resolved to fix issues such as namespacing bugs, which are out of scope here.

[pablobm]

@sedubois - Would you be able to try this out in your project? I'm looking out for possible incompatibilities, and your project appears to be amongst the most complex use cases for Administrate.

[sedubois]

@pablobm OK I'll try to have a look. Sorry for not answering other PRs, hopefully I'll find time in the next couple of weeks.

[c4lliope]

Your changes look incredibly good; could you rebase?

It seems like your test coverage is more than enough to catch any bugs people would run into. While it would be nice if @sedubois is able to test this in his application, I don't think we should let his non-feedback block us from merging in your changes. This is a change that much simplifies the usage of Assemble, and our goal is being as simple as possible for developers to build on.

Really nice going, I'm behind you 💯 .

[pablobm]

@c4lliope, thank you for your review. I have made some changes, but also left a couple of questions to see what you think. Additionally, I have added a couple of commits on top to change a couple of documentation-related details. It would be great to hear your thoughts again if you have the time!

[c4lliope]

All your code here looks really good!

I say 🚢.

[nickcharlton]

Tiny thing on including FactoryBot. Also our illusive tmp/pids/.keep seems to be back in here!

[sedubois]

@pablobm I've just done a quick run of this branch on my app and did not notice particular issues, except for the unrelated #1788 (undefined local variable page). For my test I've removed some .with_options(class_name: "PaperTrail::Version") and similar that were sprinked in my dashboards and things still seemed to work.

[pablobm]

Thank you all!