Skip to content

Handle nullable foreign key in multiselect#2446

Merged
mshibuya merged 4 commits into
railsadminteam:masterfrom
sogilis:handle_nullable_foreign_key_in_multiselect
Nov 3, 2015
Merged

Handle nullable foreign key in multiselect#2446
mshibuya merged 4 commits into
railsadminteam:masterfrom
sogilis:handle_nullable_foreign_key_in_multiselect

Conversation

@jibidus

@jibidus jibidus commented Oct 28, 2015

Copy link
Copy Markdown
Contributor

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

mshibuya commented Nov 3, 2015

Copy link
Copy Markdown
Member

Thanks for your work! 🍺

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

arnvald commented Nov 13, 2015

Copy link
Copy Markdown
Contributor

@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

jibidus commented Nov 16, 2015

Copy link
Copy Markdown
Contributor Author

@arnvald Let me have a look.

@jibidus

jibidus commented Nov 16, 2015

Copy link
Copy Markdown
Contributor Author

@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

arnvald commented Nov 16, 2015

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Member

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

@jibidus

jibidus commented Nov 18, 2015

Copy link
Copy Markdown
Contributor Author

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

@arnvald

arnvald commented Nov 18, 2015

Copy link
Copy Markdown
Contributor

@jibidus thanks for fixing the bug!

@grillermo

Copy link
Copy Markdown

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

jibidus commented Mar 9, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown

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