From 317ad17d39a13c2ae18f896695fd4c9e82679dbc Mon Sep 17 00:00:00 2001 From: Jibidus Date: Tue, 27 Oct 2015 22:34:43 +0100 Subject: [PATCH 1/4] Add not nullable foreign key in dummy application --- ...027181550_change_field_test_id_to_nested_field_tests.rb | 5 +++++ .../config/edit/rails_admin_config_edit_spec.rb | 7 ++++--- 2 files changed, 9 insertions(+), 3 deletions(-) create mode 100644 spec/dummy_app/db/migrate/20151027181550_change_field_test_id_to_nested_field_tests.rb diff --git a/spec/dummy_app/db/migrate/20151027181550_change_field_test_id_to_nested_field_tests.rb b/spec/dummy_app/db/migrate/20151027181550_change_field_test_id_to_nested_field_tests.rb new file mode 100644 index 0000000000..e93665b2f6 --- /dev/null +++ b/spec/dummy_app/db/migrate/20151027181550_change_field_test_id_to_nested_field_tests.rb @@ -0,0 +1,5 @@ +class ChangeFieldTestIdToNestedFieldTests < ActiveRecord::Migration + def change + change_column :nested_field_tests, :field_test_id, :integer, null: false + end +end diff --git a/spec/integration/config/edit/rails_admin_config_edit_spec.rb b/spec/integration/config/edit/rails_admin_config_edit_spec.rb index 275c05c868..dcf3716676 100644 --- a/spec/integration/config/edit/rails_admin_config_edit_spec.rb +++ b/spec/integration/config/edit/rails_admin_config_edit_spec.rb @@ -706,7 +706,8 @@ class HelpTest < Tableless describe 'nested form' do it 'works', js: true do @record = FactoryGirl.create :field_test - @record.nested_field_tests = [NestedFieldTest.create!(title: 'title 1'), NestedFieldTest.create!(title: 'title 2')] + NestedFieldTest.create! title: 'title 1', field_test: @record + NestedFieldTest.create! title: 'title 2', field_test: @record visit edit_path(model_name: 'field_test', id: @record.id) find('#field_test_comment_attributes_field .add_nested_fields').click @@ -743,7 +744,7 @@ class HelpTest < Tableless end end @record = FieldTest.create - @record.nested_field_tests << NestedFieldTest.create!(title: 'title 1') + NestedFieldTest.create! title: 'title 1', field_test: @record visit edit_path(model_name: 'field_test', id: @record.id) expect(find('#field_test_nested_field_tests_attributes_0_title_field')).to have_content('NestedFieldTest') end @@ -776,7 +777,7 @@ class HelpTest < Tableless it 'does not show destroy button except for newly created when :allow_destroy is false' do @record = FieldTest.create - @record.nested_field_tests << NestedFieldTest.create!(title: 'nested title 1') + NestedFieldTest.create! title: 'nested title 1', field_test: @record allow(FieldTest.nested_attributes_options).to receive(:[]).with(:nested_field_tests). and_return(allow_destroy: false, update_only: false) visit edit_path(model_name: 'field_test', id: @record.id) From 496580390cb47f6f696dca4a11ef4aa8021ee39b Mon Sep 17 00:00:00 2001 From: Jibidus Date: Tue, 27 Oct 2015 22:34:50 +0100 Subject: [PATCH 2/4] Implement Association#foreign_key_nullable? --- .../adapters/active_record/association.rb | 5 +++++ lib/rails_admin/adapters/mongoid/association.rb | 5 +++++ .../adapters/active_record/association_spec.rb | 15 +++++++++++++++ .../adapters/mongoid/association_spec.rb | 7 +++++++ 4 files changed, 32 insertions(+) diff --git a/lib/rails_admin/adapters/active_record/association.rb b/lib/rails_admin/adapters/active_record/association.rb index db7a281c3c..cf97884c5e 100644 --- a/lib/rails_admin/adapters/active_record/association.rb +++ b/lib/rails_admin/adapters/active_record/association.rb @@ -37,6 +37,11 @@ def foreign_key association.foreign_key.to_sym end + def foreign_key_nullable? + return if foreign_key.nil? || type != :has_many + klass.columns_hash[association.foreign_key].null + end + def foreign_type options[:foreign_type].try(:to_sym) || :"#{name}_type" if options[:polymorphic] end diff --git a/lib/rails_admin/adapters/mongoid/association.rb b/lib/rails_admin/adapters/mongoid/association.rb index c62951a673..ea288e6371 100644 --- a/lib/rails_admin/adapters/mongoid/association.rb +++ b/lib/rails_admin/adapters/mongoid/association.rb @@ -48,6 +48,11 @@ def foreign_key association.foreign_key.to_sym rescue nil end + def foreign_key_nullable? + return if foreign_key.nil? + true + end + def foreign_type return unless polymorphic? && [:referenced_in, :belongs_to].include?(macro) association.inverse_type.try(:to_sym) || :"#{name}_type" diff --git a/spec/rails_admin/adapters/active_record/association_spec.rb b/spec/rails_admin/adapters/active_record/association_spec.rb index 0d91ad2e3d..d73b68b94f 100644 --- a/spec/rails_admin/adapters/active_record/association_spec.rb +++ b/spec/rails_admin/adapters/active_record/association_spec.rb @@ -103,6 +103,7 @@ class ARComment < ActiveRecord::Base expect(association.type).to eq :has_many expect(association.klass).to eq Division expect(association.read_only?).to be_falsey + expect(association.foreign_key_nullable?).to be_truthy end end @@ -113,6 +114,7 @@ class ARComment < ActiveRecord::Base expect(association.type).to eq :has_many expect(association.klass).to eq Team expect(association.read_only?).to be_truthy + expect(association.foreign_key_nullable?).to be_truthy end end @@ -127,6 +129,18 @@ class ARComment < ActiveRecord::Base end end + describe 'has_many association with not nullable foreign key' do + let(:field_test) { RailsAdmin::AbstractModel.new(FieldTest) } + + context 'for direct has many' do + let(:association) { field_test.associations.detect { |a| a.name == :nested_field_tests } } + + it 'returns correct values' do + expect(association.foreign_key_nullable?).to be_falsey + end + end + end + describe 'has_and_belongs_to_many association' do subject { @post.associations.detect { |a| a.name == :a_r_categories } } @@ -135,6 +149,7 @@ class ARComment < ActiveRecord::Base expect(subject.klass).to eq ARCategory expect(subject.primary_key).to eq :id expect(subject.foreign_type).to be_nil + expect(subject.foreign_key_nullable?).to be_nil expect(subject.as).to be_nil expect(subject.polymorphic?).to be_falsey expect(subject.inverse_of).to be_nil diff --git a/spec/rails_admin/adapters/mongoid/association_spec.rb b/spec/rails_admin/adapters/mongoid/association_spec.rb index cd489bc7be..ea38b3b017 100644 --- a/spec/rails_admin/adapters/mongoid/association_spec.rb +++ b/spec/rails_admin/adapters/mongoid/association_spec.rb @@ -87,6 +87,7 @@ class MongoNote expect(subject.klass).to eq MongoBlog expect(subject.primary_key).to eq :_id expect(subject.foreign_key).to eq :mongo_blog_id + expect(subject.foreign_key_nullable?).to be_truthy expect(subject.foreign_type).to be_nil expect(subject.foreign_inverse_of).to be_nil expect(subject.as).to be_nil @@ -111,6 +112,7 @@ class MongoNote expect(subject.klass).to eq MongoPost expect(subject.primary_key).to eq :_id expect(subject.foreign_key).to eq :mongo_blog_id + expect(subject.foreign_key_nullable?).to be_truthy expect(subject.foreign_type).to be_nil expect(subject.foreign_inverse_of).to be_nil expect(subject.as).to be_nil @@ -130,6 +132,7 @@ class MongoNote expect(subject.klass).to eq MongoCategory expect(subject.primary_key).to eq :_id expect(subject.foreign_key).to eq :mongo_category_ids + expect(subject.foreign_key_nullable?).to be_truthy expect(subject.foreign_type).to be_nil expect(subject.foreign_inverse_of).to be_nil expect(subject.as).to be_nil @@ -150,6 +153,7 @@ class MongoNote expect(subject.klass).to eq [MongoBlog, MongoPost] expect(subject.primary_key).to eq :_id expect(subject.foreign_key).to eq :commentable_id + expect(subject.foreign_key_nullable?).to be_truthy expect(subject.foreign_type).to eq :commentable_type expect(subject.foreign_inverse_of).to be_nil expect(subject.as).to be_nil @@ -170,6 +174,7 @@ class MongoNote expect(subject.klass).to eq MongoComment expect(subject.primary_key).to eq :_id expect(subject.foreign_key).to eq :commentable_id + expect(subject.foreign_key_nullable?).to be_truthy expect(subject.foreign_type).to be_nil expect(subject.foreign_inverse_of).to be_nil expect(subject.as).to eq :commentable @@ -194,6 +199,7 @@ class MongoNote expect(subject.klass).to eq MongoNote expect(subject.primary_key).to eq :_id expect(subject.foreign_key).to be_nil + expect(subject.foreign_key_nullable?).to be_nil expect(subject.foreign_type).to be_nil expect(subject.foreign_inverse_of).to be_nil expect(subject.as).to be_nil @@ -213,6 +219,7 @@ class MongoNote expect(subject.klass).to eq MongoNote expect(subject.primary_key).to eq :_id expect(subject.foreign_key).to be_nil + expect(subject.foreign_key_nullable?).to be_nil expect(subject.foreign_type).to be_nil expect(subject.as).to be_nil expect(subject.polymorphic?).to be_falsey From 7383a11525d8c8d5c0148b3dc138e383f1481f5d Mon Sep 17 00:00:00 2001 From: Jibidus Date: Tue, 27 Oct 2015 19:10:29 +0100 Subject: [PATCH 3/4] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c80976f485..ee0b841238 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ ### Fixed - Add button for nested-many form in modal disappeared on click([#2372](https://github.com/sferik/rails_admin/issues/2372), [#2383](https://github.com/sferik/rails_admin/pull/2383)) +- Prevent users to remove association's element when foreign key is not nullable ([#2446](https://github.com/sferik/rails_admin/issues/2446)) ## [0.7.0](https://github.com/sferik/rails_admin/tree/v0.7.0) - 2015-08-16 From c2f12afac231a7587df1539ae012dd0b1bb466e1 Mon Sep 17 00:00:00 2001 From: Jibidus Date: Tue, 27 Oct 2015 19:22:50 +0100 Subject: [PATCH 4/4] Prevent setting null to not null foreign key --- .../rails_admin/ra.filtering-multiselect.js | 44 +++++++++++-------- .../_form_filtering_multiselect.html.haml | 1 + lib/rails_admin/config/fields/association.rb | 5 +++ .../edit/rails_admin_config_edit_spec.rb | 38 ++++++++++++++++ 4 files changed, 69 insertions(+), 19 deletions(-) diff --git a/app/assets/javascripts/rails_admin/ra.filtering-multiselect.js b/app/assets/javascripts/rails_admin/ra.filtering-multiselect.js index f90bab2110..17c18f0e69 100644 --- a/app/assets/javascripts/rails_admin/ra.filtering-multiselect.js +++ b/app/assets/javascripts/rails_admin/ra.filtering-multiselect.js @@ -17,6 +17,7 @@ return { query: query }; }, sortable: false, + removable: true, regional: { up: "Up", down: "Down", @@ -78,10 +79,13 @@ this.add = $('' + this.options.regional.add + ''); + this.columns.center.append(this.add); - this.remove = $('' + this.options.regional.remove + ''); + if (this.options.removable) { + this.remove = $('' + this.options.regional.remove + ''); + this.columns.center.append(this.remove); + } - this.columns.center.append(this.add).append(this.remove) if (this.options.sortable) { this.up = $('' + this.options.regional.up + ''); this.down = $('' + this.options.regional.down + ''); @@ -89,13 +93,13 @@ } this.selection = $(''); + this.columns.right.append(this.selection); - - this.removeAll = $('' + this.options.regional.clearAll + ''); - - this.columns.right.append(this.selection) - .append(this.removeAll); + if (this.options.removable) { + this.removeAll = $('' + this.options.regional.clearAll + ''); + this.columns.right.append(this.removeAll); + } this.selection.wrap('
'); @@ -127,19 +131,21 @@ widget.selection.trigger('change'); }); - /* Remove all from selection */ - this.removeAll.click(function(e){ - widget._deSelect($('option', widget.selection)); - e.preventDefault(); - widget.selection.trigger('change'); - }); + if (this.options.removable) { + /* Remove all from selection */ + this.removeAll.click(function(e){ + widget._deSelect($('option', widget.selection)); + e.preventDefault(); + widget.selection.trigger('change'); + }); - /* Remove from selection */ - this.remove.click(function(e){ - widget._deSelect($(':selected', widget.selection)); - e.preventDefault(); - widget.selection.trigger('change'); - }); + /* Remove from selection */ + this.remove.click(function(e){ + widget._deSelect($(':selected', widget.selection)); + e.preventDefault(); + widget.selection.trigger('change'); + }); + } var timeout = null; if(this.options.sortable) { diff --git a/app/views/rails_admin/main/_form_filtering_multiselect.html.haml b/app/views/rails_admin/main/_form_filtering_multiselect.html.haml index 8e59bd7b13..7f74ddfd93 100644 --- a/app/views/rails_admin/main/_form_filtering_multiselect.html.haml +++ b/app/views/rails_admin/main/_form_filtering_multiselect.html.haml @@ -26,6 +26,7 @@ :'edit-url' => (authorized?(:edit, config.abstract_model) ? edit_path(model_name: config.abstract_model.to_param, id: '__ID__') : ''), remote_source: index_path(config.abstract_model, source_object_id: form.object.id, source_abstract_model: source_abstract_model.to_param, associated_collection: field.name, current_action: current_action, compact: true), sortable: !!field.orderable, + removable: !!field.removable, cacheAll: !!field.associated_collection_cache_all, regional: { chooseAll: t("admin.misc.chose_all"), diff --git a/lib/rails_admin/config/fields/association.rb b/lib/rails_admin/config/fields/association.rb index 2d77545d05..18a7209434 100644 --- a/lib/rails_admin/config/fields/association.rb +++ b/lib/rails_admin/config/fields/association.rb @@ -57,6 +57,11 @@ def association # rubocop:disable TrivialAccessors @associated_collection_cache_all ||= (associated_model_config.abstract_model.count < 100) end + # Reader whether association's elements can be removed + def removable + association.foreign_key_nullable? + end + # Reader for the association's child model's configuration def associated_model_config @associated_model_config ||= RailsAdmin.config(association.klass) diff --git a/spec/integration/config/edit/rails_admin_config_edit_spec.rb b/spec/integration/config/edit/rails_admin_config_edit_spec.rb index dcf3716676..589ef4229f 100644 --- a/spec/integration/config/edit/rails_admin_config_edit_spec.rb +++ b/spec/integration/config/edit/rails_admin_config_edit_spec.rb @@ -818,6 +818,44 @@ class HelpTest < Tableless end end + describe 'has_many', active_record: true do + context 'with not nullable foreign key' do + before do + RailsAdmin.config FieldTest do + edit do + field :nested_field_tests do + nested_form false + end + end + end + @field_test = FactoryGirl.create :field_test + end + + it 'don\'t allow to remove element', js: true do + visit edit_path(model_name: 'FieldTest', id: @field_test.id) + is_expected.not_to have_selector('a.ra-multiselect-item-remove') + is_expected.not_to have_selector('a.ra-multiselect-item-remove-all') + end + end + + context 'with nullable foreign key' do + before do + RailsAdmin.config Team do + edit do + field :players + end + end + @team = FactoryGirl.create :team + end + + it 'allow to remove element', js: true do + visit edit_path(model_name: 'Team', id: @team.id) + is_expected.to have_selector('a.ra-multiselect-item-remove') + is_expected.to have_selector('a.ra-multiselect-item-remove-all') + end + end + end + describe 'fields which are nullable and have AR validations', active_record: true do it 'is required' do # draft.notes is nullable and has no validation