Belongs to patch #2546

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@paultyng

paultyng commented Oct 8, 2013

Per @edestecd, see gregbell/active_admin#2082

This is the patch as suggested in the issue. The tests seem to still be passing locally.

I am attempting to write some tests, not sure what model would make the most sense to add to the test site under posts. I was originally thinking comments but that gets confusing with the built in active admin comments. Any other suggestions?

@paultyng

This comment has been minimized.

Show comment Hide comment
@paultyng

paultyng Oct 8, 2013

One thing I've noticed is that if you alias the parent, you have to specify the classname in the belongs to options, I'm not sure if thats related to this patch or not.

paultyng commented Oct 8, 2013

One thing I've noticed is that if you alias the parent, you have to specify the classname in the belongs to options, I'm not sure if thats related to this patch or not.

@paultyng

This comment has been minimized.

Show comment Hide comment
@paultyng

paultyng Oct 8, 2013

It also seems like the route method name generation for the index on the grandchild is incorrect, it only contains the immediate parent in the method name it attempts to use. For example, assuming the hierarchy: Account -> Project -> Issue, on the issue index you would see "Undefined method new_admin_project_issue_path"

paultyng commented Oct 8, 2013

It also seems like the route method name generation for the index on the grandchild is incorrect, it only contains the immediate parent in the method name it attempts to use. For example, assuming the hierarchy: Account -> Project -> Issue, on the issue index you would see "Undefined method new_admin_project_issue_path"

@zorab47

This comment has been minimized.

Show comment Hide comment
@zorab47

zorab47 Oct 25, 2013

Contributor

I'd like to help out on the pull request. Any progress on the tests?

BTW, posts can have many statistics: hits, page views, bounce rates, etc.

Contributor

zorab47 commented Oct 25, 2013

I'd like to help out on the pull request. Any progress on the tests?

BTW, posts can have many statistics: hits, page views, bounce rates, etc.

@paultyng

This comment has been minimized.

Show comment Hide comment
@paultyng

paultyng Oct 25, 2013

Views, that's a good one. I haven't done much yet, feel free to fork and have at it, I may have some time to revisit this weekend.

Views, that's a good one. I haven't done much yet, feel free to fork and have at it, I may have some time to revisit this weekend.

@zorab47

This comment has been minimized.

Show comment Hide comment
@zorab47

zorab47 Oct 25, 2013

Contributor

Is the intent to allow nesting of belongs_to calls, which would create deeper nested resources? The Inherited Resources guide recommends against deeply nesting resources.

My preference is to stick with the IR method of providing both a belongs_to for nesting and nested_belongs_to for deeper nesting. In most cases, adding the deeper nesting adds complexity. One of my current apps makes use of a four level nested hierarchy. Having to load all the parent models to generate a URL would become cumbersome and add model coupling to the views.

So the question becomes: what is the limitation we're solving? One part of it is properly creating and linking breadcrumbs. Proving the belongs_to chain would certainly be helpful for generating breadcrumbs and menu hierarchies. Should this PR become a method of integrating Inherited Resource's nested_belongs_to?


(Whew, thinking through this and explaining it is effort intensive!)

Contributor

zorab47 commented Oct 25, 2013

Is the intent to allow nesting of belongs_to calls, which would create deeper nested resources? The Inherited Resources guide recommends against deeply nesting resources.

My preference is to stick with the IR method of providing both a belongs_to for nesting and nested_belongs_to for deeper nesting. In most cases, adding the deeper nesting adds complexity. One of my current apps makes use of a four level nested hierarchy. Having to load all the parent models to generate a URL would become cumbersome and add model coupling to the views.

So the question becomes: what is the limitation we're solving? One part of it is properly creating and linking breadcrumbs. Proving the belongs_to chain would certainly be helpful for generating breadcrumbs and menu hierarchies. Should this PR become a method of integrating Inherited Resource's nested_belongs_to?


(Whew, thinking through this and explaining it is effort intensive!)

@robertjwhitney

This comment has been minimized.

Show comment Hide comment
@robertjwhitney

robertjwhitney Nov 3, 2013

Contributor

following

Contributor

robertjwhitney commented Nov 3, 2013

following

@zorab47

This comment has been minimized.

Show comment Hide comment
@zorab47

zorab47 Dec 2, 2013

Contributor

Update: this is still on my radar. I've looked through the code and determined that this will require sweeping changes, but will help to simplify the code base as well.

Contributor

zorab47 commented Dec 2, 2013

Update: this is still on my radar. I've looked through the code and determined that this will require sweeping changes, but will help to simplify the code base as well.

@amrnt

This comment has been minimized.

Show comment Hide comment
@amrnt

amrnt Dec 9, 2013

following on this

amrnt commented Dec 9, 2013

following on this

@noobguru

This comment has been minimized.

Show comment Hide comment
@noobguru

noobguru Dec 10, 2013

I've looked all over the web, is this a bug in ActiveAdmin?

It seems to mess-up foreign keys when using the belongs_to association.

for a simple setup such as
rails g scaffold user name:string nation_id:integer
rails g scaffold nation name:string population:integer
citizen belongs_to :nation
nation has_many:citizens

I keep getting this error while trying to add a user in the active admin

NoMethodError in Admin/citizens#new

Showing f:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/activeadmin-0.6.0/app/views/active_admin/resource/new.html.arb where line #1 raised:

undefined method `nation_id' for #Citizen:0x3167040

Extracted source (around line #1):

1: insert_tag renderer_for(:new)

Rails.root: f:/Rails_Projects/crudtestapp

I've looked all over the web, is this a bug in ActiveAdmin?

It seems to mess-up foreign keys when using the belongs_to association.

for a simple setup such as
rails g scaffold user name:string nation_id:integer
rails g scaffold nation name:string population:integer
citizen belongs_to :nation
nation has_many:citizens

I keep getting this error while trying to add a user in the active admin

NoMethodError in Admin/citizens#new

Showing f:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/activeadmin-0.6.0/app/views/active_admin/resource/new.html.arb where line #1 raised:

undefined method `nation_id' for #Citizen:0x3167040

Extracted source (around line #1):

1: insert_tag renderer_for(:new)

Rails.root: f:/Rails_Projects/crudtestapp

@zorab47

This comment has been minimized.

Show comment Hide comment
@zorab47

zorab47 Dec 10, 2013

Contributor

@noobguru you might want to open an issue for that and can you provide more information? How are the models setup? What do your ActiveAdmin resources contain?

Contributor

zorab47 commented Dec 10, 2013

@noobguru you might want to open an issue for that and can you provide more information? How are the models setup? What do your ActiveAdmin resources contain?

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Jan 11, 2014

Owner

What needs to happen for this?

Owner

seanlinsley commented Jan 11, 2014

What needs to happen for this?

@paultyng

This comment has been minimized.

Show comment Hide comment
@paultyng

paultyng Jan 13, 2014

I mentioned a couple of the issues with the patch above, and tests need to be written. Unfortunately I don't really have time to take a look any longer, but feel free to run with it.

I mentioned a couple of the issues with the patch above, and tests need to be written. Unfortunately I don't really have time to take a look any longer, but feel free to run with it.

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Nov 23, 2015

Owner

It's been nearly two years since this PR has seen activity, so I'm going to close it.

Owner

seanlinsley commented Nov 23, 2015

It's been nearly two years since this PR has seen activity, so I'm going to close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment