Follow-up for #3445

- Add i18n values for and/or
- Correctly handle combination of and/or, like (a AND b OR c AND d)
- Add more specs

.rubocop_todo.yml

@@ -21,7 +21,7 @@ Lint/ReturnInVoidContext:
 # Configuration parameters: CountComments, CountAsOne, ExcludedMethods, IgnoredMethods.
 # IgnoredMethods: refine
 Metrics/BlockLength:
-  Max: 1152
+  Max: 1154
 
 # Offense count: 1
 # Configuration parameters: Max, CountKeywordArgs.

app/helpers/rails_admin/main_helper.rb

@@ -52,6 +52,7 @@ def ordered_filter_options
           field.filter_options.merge(
             index: duplet[0],
             operator: filter_hash['o'] || field.default_filter_operator,
+            separator: filter_hash['s'],
             value: filter_hash['v'],
           )
         end

config/locales/rails_admin.en.yml

@@ -18,6 +18,8 @@ en:
       is_exactly: Is exactly
       starts_with: Starts with
       ends_with: Ends with
+      and: And
+      or: Or
       too_many_objects: "Too many objects, use search box above"
       no_objects: "No objects found"
       clear: Clear

lib/rails_admin/adapters/active_record.rb

@@ -176,27 +176,34 @@ def initialize(scope)
           @scope = scope
         end
 
-        def add(field, value, operator)
+        def add(field, value, operator, separator = 'OR')
+          field_statements = []
           field.searchable_columns.flatten.each do |column_infos|
             statement, value1, value2 = StatementBuilder.new(column_infos[:column], column_infos[:type], value, operator, @scope.connection.adapter_name).to_statement
-            @statements << statement if statement.present?
+            field_statements << statement if statement.present?
             @values << value1 unless value1.nil?
             @values << value2 unless value2.nil?
             table, column = column_infos[:column].split('.')
             @tables.push(table) if column
           end
-        end
+          return unless field_statements.any?
 
-        def build
-          scope = @scope.where(@statements.join(' OR '), *@values)
-          scope = scope.references(*@tables.uniq) if @tables.any?
-          scope
+          statement =
+            if field_statements.length > 1
+              "(#{field_statements.join(' OR ')})"
+            else
+              field_statements[0]
+            end
+          @statements <<
+            if @statements.any?
+              " #{separator} #{statement}"
+            else
+              statement
+            end
         end
 
-        def build_with_separator(initial_scope, separator)
+        def build
           scope = @scope.where(@statements.join, *@values)
-          initial_scope = initial_scope.references(*@tables.uniq) if @tables.any?
-          scope = @scope.or(initial_scope.where(@statements.join, *@values)) if separator == 'or'
           scope = scope.references(*@tables.uniq) if @tables.any?
           scope
         end
@@ -219,18 +226,15 @@ def query_scope(scope, query, fields = config.list.fields.select(&:queryable?))
       # filters example => {"string_field"=>{"0055"=>{"o"=>"like", "v"=>"test_value"}}, ...}
       # "0055" is the filter index, no use here. o is the operator, v the value
       def filter_scope(scope, filters, fields = config.list.fields.select(&:filterable?))
-        initial_scope = scope
         filters.each_pair do |field_name, filters_dump|
           field = fields.detect { |f| f.name.to_s == field_name }
+          wb = WhereBuilder.new(scope)
           filters_dump.each_value do |filter_dump|
-            wb = WhereBuilder.new(scope)
-
             value = parse_field_value(field, filter_dump[:v])
 
-            wb.add(field, value, (filter_dump[:o] || RailsAdmin::Config.default_search_operator))
-            scope = wb.build_with_separator(initial_scope, filter_dump[:s]) if filter_dump[:s] == 'or'
-            scope = wb.build unless filter_dump[:s] == 'or'
+            wb.add(field, value, (filter_dump[:o] || RailsAdmin::Config.default_search_operator), filter_dump[:s] == 'or' ? 'OR' : 'AND')
           end
+          scope = wb.build
         end
         scope
       end

lib/rails_admin/adapters/mongoid.rb

@@ -150,31 +150,34 @@ def query_scope(scope, query, fields = config.list.fields.select(&:queryable?))
       # filters example => {"string_field"=>{"0055"=>{"o"=>"like", "v"=>"test_value"}}, ...}
       # "0055" is the filter index, no use here. o is the operator, v the value
       def filter_scope(scope, filters, fields = config.list.fields.select(&:filterable?))
-        initial_scope = scope
         filters.each_pair do |field_name, filters_dump|
           field = fields.detect { |f| f.name.to_s == field_name }
+          statements = []
+          statement_groups = [] # To group AND conditions first
 
           filters_dump.each_value do |filter_dump|
             next unless field
 
-            statements = []
             value = parse_field_value(field, filter_dump[:v])
             conditions_per_collection = make_field_conditions(field, value, (filter_dump[:o] || 'default'))
             field_statements = make_condition_for_current_collection(field, conditions_per_collection)
             if field_statements.many?
-              statements << {'$or' => field_statements}
+              current_statement = {'$or' => field_statements}
             elsif field_statements.any?
-              statements << field_statements.first
+              current_statement = field_statements.first
+            end
+            if filter_dump[:s] == 'or'
+              statement_groups << statements
+              statements = [current_statement]
+            else
+              statements << current_statement
             end
-            separator = '$and'
-            is_separator_or_input = filter_dump[:s].present? && filter_dump[:s] == 'or'
-            scope =
-              if is_separator_or_input
-                scope.or(initial_scope.where(statements.any? ? {separator => statements} : {}))
-              else
-                scope.where(statements.any? ? {separator => statements} : {})
-              end
           end
+          if statement_groups.any?
+            statement_groups << statements
+            statements = [{'$or' => statement_groups.map { |group| {'$and' => group} }}]
+          end
+          scope = scope.and(*statements) if statements.any?
         end
         scope
       end

spec/integration/actions/index_spec.rb

@@ -341,6 +341,7 @@
           name: 'name',
           type: 'string',
           value: '',
+          separator: nil,
           operator: 'is',
           operators: %w[_discard like not_like is starts_with ends_with],
         },
@@ -350,6 +351,7 @@
           name: 'team',
           type: 'belongs_to_association',
           value: '',
+          separator: nil,
           operator: nil,
           operators: %w[_discard like not_like is starts_with ends_with _separator _present _blank],
         },

spec/integration/widgets/filter_box_spec.rb

@@ -217,4 +217,45 @@
       expect(page).to have_css '[name^="f[draft]"][name$="[v]"]'
     end
   end
+
+  describe 'for string field' do
+    let!(:players) { %w[aaa aab bbb].each { |name| FactoryBot.create :player, name: name } }
+    before do
+      RailsAdmin.config Player do
+        field :name
+        field :notes
+      end
+    end
+
+    it 'shows separators which can be used for combination method of and/or' do
+      visit index_path(model_name: 'player')
+      click_link 'Add filter'
+      click_link 'Name'
+      is_expected.not_to have_css('[name^="f[name]"][name$="[s]"]')
+      click_link 'Add filter'
+      click_link 'Name'
+      is_expected.to have_css('[name^="f[name]"][name$="[s]"]')
+      all('[name^="f[name]"][name$="[o]"] option[value="like"]').each(&:select_option)
+      all('[name^="f[name]"][name$="[v]"]').zip(%w[aa ab]).each { |elem, value| elem.set(value) }
+      click_button 'Refresh'
+      find('[name^="f[name]"][name$="[s]"] option[value="and"]').select_option
+      expect(all('td.name_field').map(&:text)).to match_array %w[aaa aab]
+      all('[name^="f[name]"][name$="[v]"]').zip(%w[aa ab]).each { |elem, value| elem.set(value) }
+      click_button 'Refresh'
+      expect(all('td.name_field').map(&:text)).to match_array %w[aab]
+      expect(find('[name^="f[name]"][name$="[s]"]').value).to eq 'and'
+    end
+
+    it 'does not add separator when adding a different field' do
+      visit index_path(model_name: 'player')
+      click_link 'Add filter'
+      click_link 'Name'
+      click_link 'Add filter'
+      click_link 'Name'
+      is_expected.to have_css('[name^="f[name]"][name$="[s]"]')
+      click_link 'Add filter'
+      click_link 'Notes'
+      is_expected.not_to have_css('[name^="f[notes]"][name$="[s]"]')
+    end
+  end
 end

spec/rails_admin/adapters/active_record_spec.rb

@@ -290,6 +290,20 @@ def parse_value(value)
         expect(abstract_model.all(filters: {'name' => {'0000' => {v: 'somewhere'}}})).to match_array @teams[2]
       end
     end
+
+    describe 'filter separators(and/or)' do
+      it 'makes correct query' do
+        expect(abstract_model.all(filters: {'name' => {'0000' => {o: 'like', v: 'some'}, '0001' => {o: 'like', s: 'or', v: 'no'}}})).to eq(@teams[2..3])
+      end
+
+      it 'does not affect filters for other fields' do
+        expect(abstract_model.all(filters: {'name' => {'0000' => {o: 'like', v: 'some'}, '0001' => {o: 'like', s: 'or', v: 'no'}}, 'division' => {'0001' => {o: 'like', v: 'bar'}}}, include: :division)).to eq(@teams[2..2])
+      end
+
+      it 'can handle (a AND b OR c AND d) expression with putting priority on AND' do
+        expect(abstract_model.all(filters: {'name' => {'0000' => {o: 'like', v: 'some'}, '0001' => {o: 'like', s: 'and', v: 'foos'}, '0002' => {o: 'like', s: 'or', v: 'nowhere'}, '0003' => {o: 'like', s: 'and', v: 'nobody'}}})).to eq(@teams[2..2])
+      end
+    end
   end
 
   describe '#build_statement' do

spec/rails_admin/adapters/mongoid_spec.rb

@@ -263,6 +263,20 @@ def parse_value(value)
         expect(@abstract_model.all(filters: {'name' => {'0000' => {o: 'like', v: 'foo'}}})).to match_array @players[2]
       end
     end
+
+    describe 'filter separators(and/or)' do
+      it 'makes correct query' do
+        expect(@abstract_model.all(filters: {'name' => {'0000' => {o: 'like', v: 'Many'}, '0001' => {o: 'like', s: 'or', v: 'Great'}}})).to eq(@players[2..3])
+      end
+
+      it 'does not affect filters for other fields' do
+        expect(@abstract_model.all(filters: {'name' => {'0000' => {o: 'like', v: 'Many'}, '0001' => {o: 'like', s: 'or', v: 'Great'}}, 'team' => {'0001' => {o: 'like', v: 'bar'}}})).to eq(@players[2..2])
+      end
+
+      it 'can handle (a AND b OR c AND d) expression with putting priority on AND' do
+        expect(@abstract_model.all(filters: {'name' => {'0000' => {o: 'like', v: 'Many'}, '0001' => {o: 'like', s: 'and', v: 'foos'}, '0002' => {o: 'like', s: 'or', v: 'Great'}, '0003' => {o: 'like', s: 'and', v: 'Nobody'}}})).to eq(@players[2..2])
+      end
+    end
   end
 
   describe '#build_statement' do
@@ -394,25 +408,25 @@ def parse_value(value)
     end
 
     it 'supports date type query' do
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: ['', '2012-01-02', '2012-01-03'], o: 'between'}}).selector).to eq('$and' => [{'date_field' => {'$gte' => Date.new(2012, 1, 2), '$lte' => Date.new(2012, 1, 3)}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: ['', '2012-01-03', ''], o: 'between'}}).selector).to eq('$and' => [{'date_field' => {'$gte' => Date.new(2012, 1, 3)}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: ['', '', '2012-01-02'], o: 'between'}}).selector).to eq('$and' => [{'date_field' => {'$lte' => Date.new(2012, 1, 2)}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: ['2012-01-02'], o: 'default'}}).selector).to eq('$and' => [{'date_field' => Date.new(2012, 1, 2)}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: [], o: 'today'}}).selector).to eq('$and' => [{'date_field' => Date.today}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: [], o: 'yesterday'}}).selector).to eq('$and' => [{'date_field' => Date.yesterday}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: [], o: 'this_week'}}).selector).to eq('$and' => [{'date_field' => {'$gte' => Date.today.beginning_of_week, '$lte' => Date.today.end_of_week}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: [], o: 'last_week'}}).selector).to eq('$and' => [{'date_field' => {'$gte' => 1.week.ago.to_date.beginning_of_week, '$lte' => 1.week.ago.to_date.end_of_week}}])
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: ['', '2012-01-02', '2012-01-03'], o: 'between'}}).selector).to eq({'date_field' => {'$gte' => Date.new(2012, 1, 2), '$lte' => Date.new(2012, 1, 3)}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: ['', '2012-01-03', ''], o: 'between'}}).selector).to eq({'date_field' => {'$gte' => Date.new(2012, 1, 3)}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: ['', '', '2012-01-02'], o: 'between'}}).selector).to eq({'date_field' => {'$lte' => Date.new(2012, 1, 2)}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: ['2012-01-02'], o: 'default'}}).selector).to eq({'date_field' => Date.new(2012, 1, 2)})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: [], o: 'today'}}).selector).to eq({'date_field' => Date.today})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: [], o: 'yesterday'}}).selector).to eq({'date_field' => Date.yesterday})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: [], o: 'this_week'}}).selector).to eq({'date_field' => {'$gte' => Date.today.beginning_of_week, '$lte' => Date.today.end_of_week}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'date_field' => {'1' => {v: [], o: 'last_week'}}).selector).to eq({'date_field' => {'$gte' => 1.week.ago.to_date.beginning_of_week, '$lte' => 1.week.ago.to_date.end_of_week}})
     end
 
     it 'supports datetime type query' do
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: ['', '2012-01-02T12:00:00', '2012-01-03T12:00:00'], o: 'between'}}).selector).to eq('$and' => [{'datetime_field' => {'$gte' => Time.zone.local(2012, 1, 2, 12), '$lte' => Time.zone.local(2012, 1, 3, 12)}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: ['', '2012-01-03T12:00:00', ''], o: 'between'}}).selector).to eq('$and' => [{'datetime_field' => {'$gte' => Time.zone.local(2012, 1, 3, 12)}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: ['', '', '2012-01-02T12:00:00'], o: 'between'}}).selector).to eq('$and' => [{'datetime_field' => {'$lte' => Time.zone.local(2012, 1, 2, 12)}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: ['2012-01-02T12:00:00'], o: 'default'}}).selector).to eq('$and' => [{'datetime_field' => Time.zone.local(2012, 1, 2, 12)}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: [], o: 'today'}}).selector).to eq('$and' => [{'datetime_field' => {'$gte' => Date.today.beginning_of_day, '$lte' => Date.today.end_of_day}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: [], o: 'yesterday'}}).selector).to eq('$and' => [{'datetime_field' => {'$gte' => Date.yesterday.beginning_of_day, '$lte' => Date.yesterday.end_of_day}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: [], o: 'this_week'}}).selector).to eq('$and' => [{'datetime_field' => {'$gte' => Date.today.beginning_of_week.beginning_of_day, '$lte' => Date.today.end_of_week.end_of_day}}])
-      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: [], o: 'last_week'}}).selector).to eq('$and' => [{'datetime_field' => {'$gte' => 1.week.ago.to_date.beginning_of_week.beginning_of_day, '$lte' => 1.week.ago.to_date.end_of_week.end_of_day}}])
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: ['', '2012-01-02T12:00:00', '2012-01-03T12:00:00'], o: 'between'}}).selector).to eq({'datetime_field' => {'$gte' => Time.zone.local(2012, 1, 2, 12), '$lte' => Time.zone.local(2012, 1, 3, 12)}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: ['', '2012-01-03T12:00:00', ''], o: 'between'}}).selector).to eq({'datetime_field' => {'$gte' => Time.zone.local(2012, 1, 3, 12)}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: ['', '', '2012-01-02T12:00:00'], o: 'between'}}).selector).to eq({'datetime_field' => {'$lte' => Time.zone.local(2012, 1, 2, 12)}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: ['2012-01-02T12:00:00'], o: 'default'}}).selector).to eq({'datetime_field' => Time.zone.local(2012, 1, 2, 12)})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: [], o: 'today'}}).selector).to eq({'datetime_field' => {'$gte' => Date.today.beginning_of_day, '$lte' => Date.today.end_of_day}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: [], o: 'yesterday'}}).selector).to eq({'datetime_field' => {'$gte' => Date.yesterday.beginning_of_day, '$lte' => Date.yesterday.end_of_day}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: [], o: 'this_week'}}).selector).to eq({'datetime_field' => {'$gte' => Date.today.beginning_of_week.beginning_of_day, '$lte' => Date.today.end_of_week.end_of_day}})
+      expect(@abstract_model.send(:filter_scope, FieldTest, 'datetime_field' => {'1' => {v: [], o: 'last_week'}}).selector).to eq({'datetime_field' => {'$gte' => 1.week.ago.to_date.beginning_of_week.beginning_of_day, '$lte' => 1.week.ago.to_date.end_of_week.end_of_day}})
     end
 
     it 'supports enum type query' do

src/rails_admin/filter-box.js

@@ -113,9 +113,11 @@ import flatpickr from "flatpickr";
         case "text":
         case "belongs_to_association":
         case "has_one_association":
-          if ($("#filters_box > div").length > 0) {
+          if (
+            $("#filters_box").children(`div[id^="${field_name}-"]`).length > 0
+          ) {
             separator_control = $(
-              '<select class="input-sm form-control"></select>'
+              '<select class="separator form-control form-control-sm"></select>'
             )
               .prop("name", separator_filter_name)
               .append(