Merge pull request #2446 from sogilis/handle_nullable_foreign_key_in_…

…multiselect Handle nullable foreign key in multiselect
railsadminteam · Nov 3, 2015 · 9dbe9ab · 9dbe9ab
2 parents f84bdd3 + c2f12af
commit 9dbe9ab
Show file tree

Hide file tree

Showing 10 changed files with 111 additions and 22 deletions.
diff --git 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

diff --git 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,24 +79,27 @@
 
 
       this.add = $('<a href="#" class="ui-icon ui-icon-circle-triangle-e ra-multiselect-item-add">' + this.options.regional.add + '</a>');
+      this.columns.center.append(this.add);
 
-      this.remove = $('<a href="#" class="ui-icon ui-icon-circle-triangle-w ra-multiselect-item-remove">' + this.options.regional.remove + '</a>');
+      if (this.options.removable) {
+        this.remove = $('<a href="#" class="ui-icon ui-icon-circle-triangle-w ra-multiselect-item-remove">' + this.options.regional.remove + '</a>');
+        this.columns.center.append(this.remove);
+      }
 
-      this.columns.center.append(this.add).append(this.remove)
       if (this.options.sortable) {
         this.up = $('<a href="#" class="ui-icon ui-icon-circle-triangle-n ra-multiselect-item-up">' + this.options.regional.up + '</a>');
         this.down = $('<a href="#" class="ui-icon ui-icon-circle-triangle-s ra-multiselect-item-down">' + this.options.regional.down + '</a>');
         this.columns.center.append(this.up).append(this.down);
       }
 
       this.selection = $('<select class="form-control ra-multiselect-selection" multiple="multiple"></select>');
+      this.columns.right.append(this.selection);
 
 
-
-      this.removeAll = $('<a href="#" class="ra-multiselect-item-remove-all"><span class="ui-icon ui-icon-circle-triangle-w"></span>' + this.options.regional.clearAll + '</a>');
-
-      this.columns.right.append(this.selection)
-                           .append(this.removeAll);
+      if (this.options.removable) {
+        this.removeAll = $('<a href="#" class="ra-multiselect-item-remove-all"><span class="ui-icon ui-icon-circle-triangle-w"></span>' + this.options.regional.clearAll + '</a>');
+        this.columns.right.append(this.removeAll);
+      }
 
       this.selection.wrap('<div class="wrapper"/>');
 
@@ -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
@@ -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/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
@@ -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/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/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
@@ -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
@@ -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)
@@ -817,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

diff --git 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
@@ -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