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

Incorrect behaviour when using non-default column names for associations #2382

Closed
awh-tokyo opened this issue Jul 29, 2013 · 13 comments
Closed

Comments

@awh-tokyo
Copy link

Hi there,

I'm building out an admin interface on top of an existing, non-rails and not-too-well-designed database, so my associations are all named non-standard things, like so:

class NameChangeRequest < ActiveRecord::Base
  establish_connection "oldsts_#{Rails.env}"
  self.table_name = 'm_name_change_request'

  belongs_to :member, primary_key: "sts_id", foreign_key: "sts_id"
end

class Member < ActiveRecord::Base
  establish_connection "oldsts_#{Rails.env}"
  self.table_name = 'm_member'
end

sts_id is the name of the primary key on each table.

Now, with a "default" Name Change Request resoure in Active Admin:

ActiveAdmin.register NameChangeRequest do
end

I get the following error:

undefined method `member_id_eq' for #Ransack::Search:0x007f37358066c8

Which suggests to me that it's ignoring that I specified that the key name for the "member" association is "sts_id" instead of "member_id".

The following resource:

ActiveAdmin.register NameChangeRequest do
  remove_filter :member
end

is fine.

@seanlinsley
Copy link
Contributor

Yep, this is the code in question:

module ActiveAdmin
  module Inputs
    class FilterSelectInput < ::Formtastic::Inputs::SelectInput
      include FilterBase

      # When it's a HABTM or has_many association, Formtastic builds "object_ids".
      # That doesn't fit our scenario, so we override it here.
      def input_name
        name = method.to_s
        name.concat '_id' if reflection
        name.concat multiple? ? '_in' : '_eq'
      end

@seanlinsley
Copy link
Contributor

An updated version would be:

def input_name
  name = method.to_s
  name.concat reflection.primary_key if reflection
  name.concat multiple? ? '_in' : '_eq'
end

@shekibobo
Copy link
Contributor

Not sure if this is still in the running for consideration, but I had a simplification that I tried to make more global in #2541 with searchable_method_name. I was a little distracted at the time, so I'm not sure what broke or why, but I could take a look at making this more robust.

@seanlinsley
Copy link
Contributor

Please do.

@shekibobo
Copy link
Contributor

@awh-tokyo I know it's been a while, but if you use the latest master branch does this problem still happen? And if so, does it still break if you change the :members filter to be as: :check_boxes?

@seanlinsley
Copy link
Contributor

@shekibobo note that we still have this line of code that statically uses ID as the primary key.

@shekibobo
Copy link
Contributor

I'm thinking the fix is just to make that line be the same as the line in the check_boxes input, and that should fix the problem. I just wanted to see if @awh-tokyo could still produce us a failing test case for it.

shekibobo added a commit to shekibobo/active_admin that referenced this issue Nov 19, 2013
@seanlinsley
Copy link
Contributor

Ah, it turns out Ransack gives this to us for free. @awh-tokyo if you use the code on master, this problem should go away:

gem 'activeadmin', github: 'gregbell/active_admin'

This little bug report has taken up way too much of my time 😓

@seanlinsley
Copy link
Contributor

An example:

gem 'rails', '3.2.16'
gem 'ransack'
require 'rails/all'
require 'ransack'

module Example
  class Application < Rails::Application
  end
end

ActiveRecord::Base.establish_connection adapter:  'sqlite3',
                                        database: ':memory:'

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.integer :id
  end

  create_table :user_actions, force: true do |t|
    t.integer :id
    t.integer :user_id
    t.string  :action_type
  end

  create_table :actions, force: true do |t|
    t.integer :id
    t.string  :action_type
    t.string  :description
  end
end

class User < ActiveRecord::Base
  has_many :user_actions
end

class Action < ActiveRecord::Base
  validates_uniqueness_of :action_type
end

class UserAction < ActiveRecord::Base
  belongs_to :user
  belongs_to :action, foreign_key: :action_type, primary_key: :action_type
end

User.create

Action.create action_type: 'foo', description: 'bar'
Action.create action_type: 'baz', description: 'bax'

UserAction.create action_type: 'foo', user_id: 1
UserAction.create action_type: 'baz', user_id: 1

UserAction.search(action_id_eq: 1).result.count # => 1
UserAction.search(action_id_eq: 2).result.count # => 1
UserAction.search(action_id_eq: 3).result.count # => 0

The SQL it uses is:

SELECT "user_actions".* FROM "user_actions" LEFT OUTER JOIN "actions"
  ON "actions"."action_type" = "user_actions"."action_type"
  WHERE "actions"."id" = 3

seanlinsley added a commit that referenced this issue Jan 20, 2014
Ransack handles it for us, so there's no need to do all that extra work
:D

Note that this commit disables part of `new_page.feature` as a
side-effect of me renaming the association's foreign key.
shekibobo added a commit to shekibobo/active_admin that referenced this issue Jan 20, 2014
@amiuhle
Copy link
Contributor

amiuhle commented Feb 23, 2016

This is still an issue for me (on master, with rails 5.0.0.beta2).

@seanlinsley seanlinsley reopened this Feb 23, 2016
@seanlinsley
Copy link
Contributor

Indeed it is:

name.concat '_id' if reflection

Here's an attempt @shekibobo made: shekibobo@3e075f5. I don't remember the context behind that, and why there wasn't a PR.

Feel free to take that code and open a PR, provided you add tests 😄

diff --git a/lib/active_admin/inputs/filter_select_input.rb b/lib/active_admin/inputs/filter_select_input.rb
index e83f1ab..72b4e4e 100644
--- a/lib/active_admin/inputs/filter_select_input.rb
+++ b/lib/active_admin/inputs/filter_select_input.rb
@@ -13,9 +13,9 @@ def searchable_method_name
         if searchable_has_many_through?
           "#{reflection.through_reflection.name}_#{reflection.foreign_key}"
         else
-          name = method.to_s
-          name.concat '_id' if reflection
-          name
+          polymorphic = reflection && reflection.macro == :belongs_to && reflection.options[:polymorphic]
+          key = polymorphic ? nil : reflection.try(:association_primary_key)
+          name = [method, key].compact.join('_')
         end
       end

@amiuhle
Copy link
Contributor

amiuhle commented Feb 24, 2016

There actually was a PR, it's linked above.

I have already gone back to the standard id:integer primary keys. But if I can find the time, I might still give that problem a shot.

@javierjulio
Copy link
Member

Closing as stale. Feel free to open a PR thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants