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

Handle nullable foreign key in multiselect #2446

Conversation

jibidus
Copy link
Contributor

@jibidus jibidus commented Oct 28, 2015

has_many with not nullable foreign may raise exception when user try to remove elements from corresponding association.
The aim of this request is to prevent such behavior with disabling remove actions from form.

@jibidus jibidus force-pushed the handle_nullable_foreign_key_in_multiselect branch 2 times, most recently from dd4ad2f to b86b269 Compare October 29, 2015 16:01
@jibidus jibidus force-pushed the handle_nullable_foreign_key_in_multiselect branch from b86b269 to c2f12af Compare October 29, 2015 16:14
@mshibuya mshibuya merged commit c2f12af into railsadminteam:master Nov 3, 2015
mshibuya added a commit that referenced this pull request Nov 3, 2015
…multiselect

Handle nullable foreign key in multiselect
@mshibuya
Copy link
Member

mshibuya commented Nov 3, 2015

Thanks for your work! 🍺

@jibidus jibidus deleted the handle_nullable_foreign_key_in_multiselect branch November 3, 2015 09:51
@arnvald
Copy link
Contributor

arnvald commented Nov 13, 2015

@jibidus @mshibuya it seems that this PR breaks has_many :through relation 😢

I've got relation:

promotion has many user_promotions
promotion has many users through user_promotions

when I'm opening view to add new promotion, I can add users there, and what happens during this code execution:

def foreign_key_nullable?
  return if foreign_key.nil? || type != :has_many
  klass.columns_hash[association.foreign_key].null
end

is that foreign_key is present (:user_id), relation type is :has_many, but klass.columns_hash does not contain "user_id" therefore it returns nil and the error is undefined method "null" for nil:NilClass

Any idea how to fix it?

arnvald added a commit to Kaligo/rails_admin that referenced this pull request Nov 13, 2015
@jibidus
Copy link
Contributor Author

jibidus commented Nov 16, 2015

@arnvald Let me have a look.

@jibidus
Copy link
Contributor Author

jibidus commented Nov 16, 2015

@arnvald I didn't manage to reproduce the issue 😩 .
When exactly the issue is raised? When you add user to new promotion ?
Do you have a RSpec test to be sure I can reproduce this issue?

@arnvald
Copy link
Contributor

arnvald commented Nov 16, 2015

@jibidus I created a simple application here: https://github.com/arnvald/rails_admin_polymorphic_bug

To reproduce:

  1. bundle install
  2. bin/rake db:create db:migrate
  3. bin/rails s
  4. go to http://localhost:3000/admin/user/new

I'll try to write a spec that'll cover this case, either today or tomorrow.

@mshibuya
Copy link
Member

@jibidus @arnvald Opened as a new issue: #2474

@jibidus
Copy link
Contributor Author

jibidus commented Nov 18, 2015

@arnvald @mshibuya Pull Request created to fix this issue: #2478.
Sorry for the issue and the delay 😟 .

@arnvald
Copy link
Contributor

arnvald commented Nov 18, 2015

@jibidus thanks for fixing the bug!

@grillermo
Copy link

How can i configure this manually? i want some has_many fields without those buttons to remove the association. But adding

    field :addresses do
      removable? do
        false
      end
    end

Does nothing.

@jibidus
Copy link
Contributor Author

jibidus commented Mar 9, 2017

This Pull Request was designed to prevent displaying the Remove button when foreign key is not nullable.
So, if make your foreign key not nullable, the Remove button will disappear.
If you don't want to make your foreign key not nullable, I think you cannot do that (hide Remove button).

@grillermo
Copy link

I made my own custom field for my use case, i don't want the foreign key nullable i wanted that field to not have that remove button on that particular model, so this is what i did:
https://gist.github.com/grillermo/9c0e5521a5e5b9361846128eb272c8c0
Thanks for the answer

christiandennis pushed a commit to glooko/rails_admin that referenced this pull request May 16, 2017
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

Successfully merging this pull request may close these issues.

4 participants