Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Nested belongs_to is broken #2082

Open
e110c0 opened this Issue Apr 9, 2013 · 24 comments

Comments

Projects
None yet

e110c0 commented Apr 9, 2013

I have the following situation:

ActiveAdmin.register Location do
end
ActiveAdmin.register Event do
    belongs_to :location
end
ActiveAdmin.register Ticket do
    belongs_to :event
end

I would expect these namespaces to work:

  • locations
  • locations/[id]
  • locations/[id]/events
  • locations/[id]/events/[id]
  • locations/[id]/events/[id]/tickets
  • locations/[id]/events/[id]/tickets/[id]

instead the following work

  • locations
  • locations/[id]
  • locations/[id]/events
  • locations/[id]/events/[id]

and (kind of surprising)

  • events/[id]/tickets
  • events/[id]/tickets/[id]

while these don't work:

  • locations/[id]/events/[id]/tickets
  • locations/[id]/events/[id]/tickets/[id]
  • events
  • events/[id]

the last two are on the one hand what I except because events are within locations namespace, but given that the namespace "events" seems to exists for the tickets, that is quite puzzling.

as expected, stuff like

admin_location_event_tickets_path(locationid, eventid)

does not work, neither. (NoMethodError)

admin_event_tickets_path(eventid)

does work and gives a valid path while

admin_event_path(eventid)

also works but gives an invalid path (shouldn't work at all in my opinion)

the current behavior also breaks the breadcumbs: if you use events/[id]/tickets, the events oder the events/[id] link in the breadcrumbs are broken

Contributor

macfanatic commented Apr 9, 2013

Duplicate of #2051.

@macfanatic macfanatic closed this Apr 9, 2013

e110c0 commented May 7, 2013

This is not the same bug. The breadcrumbs is just one part of it. the problem is a hierarchy of more than 1 belongs_to doesn't work inside activeadmin.

Owner

seanlinsley commented May 7, 2013

I hadn't noticed it before, but an experiment reproduced much the same problems that @e110c0 reported.

@seanlinsley seanlinsley reopened this May 7, 2013

eLod commented May 10, 2013

i've run into this issue, this seems to fix it:

new method added to ActiveAdmin::Resource::BelongsTo

      def chain
        [owner] + (target.belongs_to? && target.belongs_to_config.chain || [target])
      end

and replacing in ActiveAdmin::Router (in the relevant section for belongs_to in #define_resource_routes):

              # Make the nested belongs_to routes
              # :only is set to nothing so that we don't clobber any existing routes on the resource
              chain = config.belongs_to_config.chain
              chain.shift
              current = proc { instance_eval &routes_for_belongs_to }
              nested = chain.reduce(current) { |blk, resource| proc { resources resource.resource_name.plural, :only => [], &blk } }
              instance_eval &nested

              #resources config.belongs_to_config.target.resource_name.plural, :only => [] do
              #  instance_eval &routes_for_belongs_to
              #end

so the basic idea is to wrap the original block traversing up the belongs_to chain. not sure about the conditional belongs_to though (should work for the most simple cases, but if A belongs to B belongs to C and the latter is conditional A's routes should be drawn in B and in B nested in C).

please note: this only fixes how routes are drawn, e.g. inherited_resources still needs to know that this is a nested resource to generate the correct helpers (quick workaround: controller { nested_belongs_to :foo, :bar } when registering), so i'm not sure about what else would need to be updated

Owner

seanlinsley commented May 10, 2013

Thanks for the debugging work @eLod

This is going to require a comprehensive set of tests to ensure this gets and stays fixed.

Contributor

edestecd commented Oct 4, 2013

Monkey patch for 0.6.1, in case anyone else needs this:
(this also fixes broken comment redirects)

config/initializers/active_admin_nested_belongs_to.rb

 module ActiveAdmin
   class Resource
     class BelongsTo
       def chain
         [owner] + (target.belongs_to? && target.belongs_to_config.chain || [target])
       end
     end
   end
 end


module ActiveAdmin
  class Resource
    module Routes
      class RouteBuilder
        # @return params to pass to instance path
        def route_instance_params(instance)
          if nested?
            chain = resource.belongs_to_config.chain
            chain.shift
            chain.inject([instance]) { |i_params, target| i_params << i_params.last.send(target.resource_name.singular) }.reverse.map(&:to_param)
          else
            instance.to_param
          end
        end

        def route_collection_params(params)
          if nested?
            chain = resource.belongs_to_config.chain
            chain.shift
            chain.inject([]) { |r_params, target| r_params << params[:"#{target.resource_name.singular}_id"] }.reverse
          end
        end

        def belongs_to_name
          if nested?
            chain = resource.belongs_to_config.chain
            chain.shift
            chain.inject([]) { |route, target| route << target.resource_name.singular }.reverse.join('_')
          end
        end
      end
    end
  end
end


module ActiveAdmin
  class Router
    # Define the routes for each resource
    def define_resource_routes(router)
      router.instance_exec @application.namespaces, self do |namespaces, aa_router|
        resources = namespaces.values.map{ |n| n.resources.values }.flatten
        resources.each do |config|
          routes = aa_router.resource_routes(config)

          # Add in the parent if it exists
          if config.belongs_to?
            belongs_to = routes
            routes     = Proc.new do
              # If its optional, make the normal resource routes
              instance_exec &belongs_to if config.belongs_to_config.optional?

              # Make the nested belongs_to routes
              # :only is set to nothing so that we don't clobber any existing routes on the resource
              chain = config.belongs_to_config.chain
              chain.shift
              current = proc { instance_exec &belongs_to }
              nested = chain.reduce(current) { |blk, resource| proc { resources resource.resource_name.plural, :only => [], &blk } }
              instance_exec &nested

              #resources config.belongs_to_config.target.resource_name.plural, :only => [] do
              #  instance_exec &belongs_to
              #end
            end
          end

          # Add on the namespace if required
          unless config.namespace.root?
            nested = routes
            routes = Proc.new do
              namespace config.namespace.name do
                instance_exec &nested
              end
            end
          end

          instance_exec &routes
        end
      end
    end
  end
end
Owner

seanlinsley commented Oct 4, 2013

@edestecd could you open a pull request?

Contributor

edestecd commented Oct 4, 2013

Possibly, but I have no tests at this point.

Owner

seanlinsley commented Oct 5, 2013

All good things in time.

paultyng added a commit to paultyng/active_admin that referenced this issue Oct 8, 2013

Contributor

robertjwhitney commented Oct 30, 2013

Any tell on when this might get reviewed for merge?

Owner

seanlinsley commented Oct 30, 2013

If you're interested in this getting fixed, feel free to chime in on the pull request itself: #2546

Contributor

bamorim commented Jul 15, 2014

#3274 Had the same problem
#3276 Fixed this here

Contributor

stereoscott commented Aug 1, 2014

updated pull request --> #3280

@edestecd @bamorim maybe you have a monkey patch working for 1.0.0.pre ??

It would be nice to have this working at some point, it has been 3 years since this was reported. Did someone get it working and could tell us what needs to be done?

Contributor

edestecd commented Apr 14, 2016

I can share my most recent monkey patch...
I should be able to dig it up this weekend.

@edestecd I would be really grateful if you could share it for now.

Contributor

edestecd commented Apr 21, 2016

This is what I had just before Rails 4. I removed it when upgrading. I suspect that it was mostly working in the new version and the patch is no longer needed. Is this still not working for some? I'd say close the issue, unless someone reports issue still.

config/initializers/active_admin_nested_belongs_to.rb

# Nested belongs_to does not work in activeadmin-0.6.0
# This is the suggested hack.
# Maybe it will make it into the next version.
# https://github.com/gregbell/active_admin/issues/2082
#
# This version is now updated for activeadmin-0.6.1
#

module ActiveAdmin
  class Resource
    class BelongsTo
      def chain
        [owner] + (target.belongs_to? && target.belongs_to_config.chain || [target])
      end
    end
  end
end

module ActiveAdmin
  class Resource
    module Routes
      class RouteBuilder
        # @return params to pass to instance path
        def route_instance_params(instance)
          if nested?
            chain = resource.belongs_to_config.chain
            chain.shift
            chain.reduce([instance]) { |i_params, target| i_params << i_params.last.send(target.resource_name.singular) }.reverse.map(&:to_param)
          else
            instance.to_param
          end
        end

        def route_collection_params(params)
          return unless nested?
          chain = resource.belongs_to_config.chain
          chain.shift
          chain.reduce([]) { |r_params, target| r_params << params[:"#{target.resource_name.singular}_id"] }.reverse
        end

        def belongs_to_name
          return unless nested?
          chain = resource.belongs_to_config.chain
          chain.shift
          chain.reduce([]) { |route, target| route << target.resource_name.singular }.reverse.join('_')
        end
      end
    end
  end
end

module ActiveAdmin
  class Router
    # Define the routes for each resource
    def define_resource_routes(router)
      router.instance_exec @application.namespaces, self do |namespaces, aa_router|
        resources = namespaces.values.map { |n| n.resources.values }.flatten
        resources.each do |config|
          routes = aa_router.resource_routes(config)

          # Add in the parent if it exists
          if config.belongs_to?
            belongs_to = routes
            routes     = proc do
              # If its optional, make the normal resource routes
              instance_exec(&belongs_to) if config.belongs_to_config.optional?

              # Make the nested belongs_to routes
              # :only is set to nothing so that we don't clobber any existing routes on the resource
              chain = config.belongs_to_config.chain
              chain.shift
              current = proc { instance_exec(&belongs_to) }
              nested = chain.reduce(current) { |blk, resource| proc { resources resource.resource_name.plural, only: [], &blk } }
              instance_exec(&nested)

              # resources config.belongs_to_config.target.resource_name.plural, :only => [] do
              #   instance_exec &belongs_to
              # end
            end
          end

          # Add on the namespace if required
          unless config.namespace.root?
            nested = routes
            routes = proc do
              namespace config.namespace.name do
                instance_exec(&nested)
              end
            end
          end

          instance_exec(&routes)
        end
      end
    end
  end
end

While I have yet to try your code I find that you can hardly say it's working.

For example

ActiveAdmin.register Project do
end
ActiveAdmin.register Report do
    belongs_to :project
end
ActiveAdmin.register Issues do
    belongs_to :report
end

Still generates /admin/reports/:report_id/issues path. The breadcrumbs then offer you to go to /admin/reports which don't exist as you could already guess.

Since the bug isn't fixed, a little hack can be done as following:

Instead of having a route like: locations/[id]/events/[id]/tickets, we can have a link to /tickets?q[event_id_eq]=[id] so that we set a filter on the ticket's event id.

link_to 'Tickets', admin_tickets_path(q: { event_id_eq: ticket.id }) assuming we have a var ticket containing the right ticket.

Hope this can help

For 1.0.4.pre, no hack is needed, just a little tweak coming from the inherited_resource documents and you're set :)

ActiveAdmin.register Store do
  sidebar "Store Details", only: [:show, :edit] do
    ul do
      li link_to "Deals",    admin_store_deals_path(resource)
      li link_to "Products",    admin_store_products_path(resource)
    end
  end
end 

ActiveAdmin.register Deal do
  belongs_to :store, optional: true

    sidebar "Deal Details", only: [:show, :edit] do
    ul do
      li link_to "Products",    admin_deal_products_path(resource)
    end
  end
end

ActiveAdmin.register Product do
  belongs_to :deal, optional: true
  belongs_to :store, optional: true

  controller do
    nested_belongs_to :store, :deal, optional: true
  end
end


class Store < ApplicationRecord
  has_many :deals
  has_many :products, through: :deals
end

class Deal < ApplicationRecord
  belongs_to :store
  has_many :products
end

class Product < ApplicationRecord
  belongs_to :deal
end
Owner

varyonic commented Jan 24, 2017

Can someone opine if this is resolved by #4618/#3280?

axorinc commented Apr 20, 2017 edited

Is there any solution for this problem, For versions 1.0.0pre4 or higher?

xn commented Jul 24, 2017

@varyonic, this still seems to be a problem for me.

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