search

mountetna / magma

Data server with friendly data loaders
GNU General Public License v2.0
5 stars 2 forks source link

Add support for link_model on attributes table #134

Closed notmarkmiranda closed 4 years ago

notmarkmiranda commented 4 years ago

127

This will add a link_model attribute to the attributes table. It also adds :link_model to the EDITABLE_OPTIONS of attribute.

graft commented 4 years ago

Hmm, is this all it takes now to have the link_model load from the attribute table? If so, very cool. Would be nice to have some specs validating this, not sure how to set that up...

alimi commented 4 years ago

We could cover it in one or all of these tests:

https://github.com/mountetna/magma/blob/b9ce3a1028847ece9239d841e5fe0236a0ee8024/spec/magma_attribute_spec.rb#L5-L43

Everything in EDITABLE_OPTIONS should behave the same though so I'm not sure we want to test every option.

graft commented 4 years ago

I think you are conflicting with the behavior of Magma::Link#link_model, a module which is included into some Magma attributes. I have some regrets about writing this module, but this method is used extensively throughout Magma so it would be pretty tough to change. The method uses the instance variable @link_modelwhich is set on the model, but itself returns the model class, not a string. This behavior is different from the attr_reader :link_model you defined, which returns a string. This doesn't explode tests because include Magma::Link appears to overwrite the attr_reader :link_model, but it seems like this problem should be avoided by renaming @link_model to @link_model_name and changing the attr_reader (and maybe the column name in the table?) appropriately.

Since this is the main thing we to ensure we can do (successfully link to another model), it might be nice to split this out from the other options test, and to fully exercise the behavior of Magma::Link#link_model starting from the attribute fixture, and using some sort of link attribute (like child or collection attribute) instead of a generic Magma::Attribute. Also, I think link_model_name should be formatted like model_name (that is, snake_case) rather than holding a ruby class name.

Also: I think somewhere on the way this thing should be raising an exception if the link_model_name does not exist in the project - maybe using Magma.instance.get_model? A test proving this would also be useful.

notmarkmiranda commented 4 years ago

Merging this to avoid scope creep and leaving the other refactors for another time. Not wanting to block progress on other things.