prepare the query before reading results (#5547)

* prepare the query before reading results

* use string for list cf filter value

because all other list filter also use a string

* handle invalid group, columns, sort on query read

[ci skip]

app/models/queries/filters/base.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
@@ -35,7 +36,7 @@ class Queries::Filters::Base
   class_attribute :model,
                   :filter_params
 
-  self.filter_params = [:operator, :values]
+  self.filter_params = %i(operator values)
 
   attr_accessor :context,
                 *filter_params
@@ -73,6 +74,10 @@ class Queries::Filters::Base
     nil
   end
 
+  def valid_values!
+    type_strategy.valid_values!
+  end
+
   def available?
     true
   end

app/models/queries/filters/strategies/base_strategy.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
@@ -52,6 +53,8 @@ module Queries::Filters::Strategies
         .slice(*self.class.supported_operators)[filter.operator]
     end
 
+    def valid_values!; end
+
     def supported_operator_classes
       operator_map
         .slice(*self.class.supported_operators)

app/models/queries/filters/strategies/list.rb

@@ -43,5 +43,9 @@ module Queries::Filters::Strategies
         errors.add(:values, :inclusion)
       end
     end
+
+    def valid_values!
+      filter.values &= (allowed_values.map(&:last) + ['-1'])
+    end
   end
 end

app/models/queries/filters/strategies/list_optional.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)

app/models/queries/work_packages/filter/author_filter.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)

app/models/queries/work_packages/filter/category_filter.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
@@ -29,7 +30,6 @@
 
 class Queries::WorkPackages::Filter::CategoryFilter <
   Queries::WorkPackages::Filter::WorkPackageFilter
-
   def allowed_values
     all_project_categories.map { |s| [s.name, s.id.to_s] }
   end

app/models/queries/work_packages/filter/custom_field_filter.rb

@@ -37,7 +37,7 @@ class Queries::WorkPackages::Filter::CustomFieldFilter <
   def allowed_values
     case custom_field.field_format
     when 'list'
-      custom_field.custom_options.map { |co| [co.value, co.id] }
+      custom_field.custom_options.map { |co| [co.value, co.id.to_s] }
     when 'bool'
       [[I18n.t(:general_text_yes), CustomValue::BoolStrategy::DB_VALUE_TRUE],
        [I18n.t(:general_text_no), CustomValue::BoolStrategy::DB_VALUE_FALSE]]

app/models/queries/work_packages/filter/status_filter.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)

app/models/query.rb

@@ -231,6 +231,25 @@ class Query < ActiveRecord::Base
     end
   end
 
+  # Try to fix an invalid query
+  #
+  # Fixes:
+  # * filters:
+  #     Reduces the filter's values to those that are valid.
+  #     If the filter remains invalid, it is removed.
+  # * group_by:
+  #     Removes the group by if it is invalid
+  # * sort_criteria
+  #     Removes all invalid criteria
+  #
+  # If the query has been valid or if the error
+  # is not one of the addressed, the query is unchanged.
+  def valid_subset!
+    valid_filter_subset!
+    valid_group_by_subset!
+    valid_sort_criteria_subset!
+  end
+
   def add_filter(field, operator, values)
     filter = filter_for(field)
 
@@ -526,4 +545,30 @@ class Query < ActiveRecord::Base
       filters
     end
   end
+
+  def valid_filter_subset!
+    filters.each do |filter|
+      filter.valid_values!
+
+      if filter.invalid?
+        filters.delete(filter)
+      end
+    end
+  end
+
+  def valid_group_by_subset!
+    unless groupable_columns.map(&:name).map(&:to_s).include?(group_by.to_s)
+      self.group_by = nil
+    end
+  end
+
+  def valid_sort_criteria_subset!
+    available_criteria = sortable_columns.map(&:name).map(&:to_s)
+
+    sort_criteria.each do |criteria|
+      unless available_criteria.include? criteria.first.to_s
+        sort_criteria.delete(criteria)
+      end
+    end
+  end
 end

lib/api/v3/queries/queries_api.rb

@@ -115,6 +115,15 @@ module API
             end
 
             get do
+              # We try to ignore invalid aspects of the query as the user
+              # might not even be able to fix them (public  query)
+              # and because they might only be invalid in his context
+              # but not for somebody having more permissions, e.g. subproject
+              # filter for admin vs for anonymous.
+              # Permissions are enforced nevertheless.
+              @query.valid_subset!
+
+              # We do not ignore invalid params provided by the client.
               query_representer_response(@query, params)
             end
 

lib/api/v3/queries/query_helper.rb

@@ -38,9 +38,15 @@ module API
         # @param query [Query]
         # @param contract [Class]
         # @param form_representer [Class]
+        #
+        # Additionally, two parameters are accepted under the hood.
+        #
+        # # request_body
+        # # params
+        #
+        # Both are applied to the query in order to adapt it.
         def create_or_update_query_form(query, contract, form_representer)
-          representer = ::API::V3::Queries::QueryRepresenter.create query, current_user: current_user
-          query = representer.from_hash Hash(request_body)
+          query = update_query_from_body_and_params(query, request_body, params)
           contract = contract.new query, current_user
           contract.validate
 
@@ -97,6 +103,15 @@ module API
           # the service mutates the given query in place so we just return it
           query
         end
+
+        def update_query_from_body_and_params(query, body, params)
+          representer = ::API::V3::Queries::QueryRepresenter.create query, current_user: current_user
+          # Note that we do not deal with failures here. The query
+          # needs to be validated later.
+          UpdateQueryFromV3ParamsService.new(query, current_user).call(params)
+
+          representer.from_hash Hash(body)
+        end
       end
     end
   end

spec/features/work_packages/table/invalid_query_spec.rb

@@ -16,32 +16,88 @@ describe 'Invalid query spec', js: true do
   let(:status) do
     FactoryGirl.create(:status)
   end
+  let(:status2) do
+    FactoryGirl.create(:status)
+  end
 
-  let(:query) do
+  let(:invalid_query) do
     query = FactoryGirl.create(:query,
                                project: project,
                                user: user)
 
     query.add_filter('assigned_to_id', '=', [99999])
-    query.save!(validate: false)
+    query.columns << 'cf_0815'
+    query.group_by = 'cf_0815'
+    query.sort_criteria = [['cf_0815', 'desc']]
+    query.save(validate: false)
 
     query
   end
 
+  let(:valid_query) do
+    FactoryGirl.create(:query,
+                       project: project,
+                       user: user)
+  end
+
+  let(:work_package_assigned) do
+    FactoryGirl.create(:work_package,
+                       project: project,
+                       assigned_to: user)
+  end
+
   before do
     login_as(user)
     status
+    status2
     member
-
-    wp_table.visit_query(query)
+    work_package_assigned
   end
 
   # Regression test for bug #24114 (broken watcher filter)
-  it 'should load the faulty query' do
-    expect(page).to have_selector(".notification-box.-error", text: I18n.t('js.work_packages.faulty_query.description'), wait: 10)
+  it 'should load a faulty query' do
+    wp_table.visit_query(invalid_query)
+
+    filters.open
+    filters.expect_filter_count 1
+    filters.expect_no_filter_by('Assignee')
+    filters.expect_filter_by('Status', 'open', nil)
+
+    wp_table.expect_no_notification(type: :error,
+                                    message: I18n.t('js.work_packages.faulty_query.description'))
+
+    wp_table.expect_work_package_listed [work_package_assigned]
+  end
+
+  it 'should not load with faulty parameters but can be fixed' do
+    filter_props = [{ 'n': 'assignee', 'o': '=', 'v': ['999999'] },
+                    { 'n': 'status', 'o': '=', 'v': [status.id.to_s, status2.id.to_s] }]
+    column_props = ['id', 'subject', 'customField0815']
+    invalid_props = JSON.dump('f': filter_props,
+                              'c': column_props,
+                              'g': 'customField0815',
+                              't': 'customField0815:desc')
+
+    wp_table.visit_with_params("query_id=#{valid_query.id}&query_props=#{invalid_props}")
 
     filters.open
     filters.expect_filter_count 2
-    expect(page).to have_select('values-assignee', selected: I18n.t('js.placeholders.selection'))
+    filters.expect_filter_by('Assignee', 'is', I18n.t('js.placeholders.selection'))
+    filters.expect_filter_by('Status', 'is', [status.name, status2.name])
+
+    wp_table.expect_notification(type: :error,
+                                 message: I18n.t('js.work_packages.faulty_query.description'))
+
+    wp_table.expect_no_work_package_listed
+
+    wp_table.group_by('Assignee')
+    sleep(0.1)
+    filters.set_filter('Assignee', 'is', user.name)
+    sleep(0.1)
+
+    wp_table.expect_work_package_listed [work_package_assigned]
+    wp_table.save
+
+    wp_table.expect_notification(message: I18n.t('js.notice_successful_update'))
   end
 end

spec/models/queries/work_packages/filter/assigned_to_filter_spec.rb

@@ -152,4 +152,40 @@ describe Queries::WorkPackages::Filter::AssignedToFilter, type: :model do
       end
     end
   end
+
+  it_behaves_like 'basic query filter' do
+    let(:order) { 4 }
+    let(:type) { :list_optional }
+    let(:class_key) { :assigned_to_id }
+
+    describe '#valid_values!' do
+      let(:user) { FactoryGirl.build_stubbed(:user) }
+      let(:loader) do
+        loader = double('loader')
+
+        allow(loader)
+          .to receive(:user_values)
+          .and_return([[user.name, user.id.to_s]])
+        allow(loader)
+          .to receive(:group_values)
+          .and_return([])
+
+        loader
+      end
+
+      before do
+        allow(::Queries::WorkPackages::Filter::PrincipalLoader)
+          .to receive(:new)
+          .and_return(loader)
+
+        instance.values = [user.id.to_s, '99999']
+      end
+
+      it 'remove the invalid value' do
+        instance.valid_values!
+
+        expect(instance.values).to match_array [user.id.to_s]
+      end
+    end
+  end
 end

spec/models/queries/work_packages/filter/custom_field_filter_spec.rb

@@ -295,7 +295,7 @@ describe Queries::WorkPackages::Filter::CustomFieldFilter, type: :model do
     it 'is list_optional for a list' do
       instance.name = "cf_#{list_wp_custom_field.id}"
       expect(instance.allowed_values)
-        .to match_array(list_wp_custom_field.custom_options.map { |co| [co.value, co.id] })
+        .to match_array(list_wp_custom_field.custom_options.map { |co| [co.value, co.id.to_s] })
     end
 
     it 'is list_optional for a user' do

spec/models/queries/work_packages/filter/status_filter_spec.rb

@@ -68,6 +68,22 @@ describe Queries::WorkPackages::Filter::StatusFilter, type: :model do
       end
     end
 
+    describe '#valid_values!' do
+      before do
+        allow(Status)
+          .to receive(:all)
+          .and_return [status]
+
+        instance.values = [status.id.to_s, '99999']
+      end
+
+      it 'remove the invalid value' do
+        instance.valid_values!
+
+        expect(instance.values).to match_array [status.id.to_s]
+      end
+    end
+
     describe '#value_objects' do
       before do
         allow(Status)

spec/models/queries/work_packages/filter/subject_filter_spec.rb

@@ -46,6 +46,17 @@ describe Queries::WorkPackages::Filter::SubjectFilter, type: :model do
       end
     end
 
+    describe '#valid_values!' do
+      it 'is a noop' do
+        instance.values = ['none', 'is', 'changed']
+
+        instance.valid_values!
+
+        expect(instance.values)
+          .to match_array ['none', 'is', 'changed']
+      end
+    end
+
     it_behaves_like 'non ar filter'
   end
 end

spec/models/query_spec.rb

@@ -190,6 +190,106 @@ describe Query, type: :model do
     end
   end
 
+  describe '#valid_subset!' do
+    let(:valid_status) { FactoryGirl.build_stubbed(:status) }
+
+    context 'filters' do
+      before do
+        allow(Status)
+          .to receive(:all)
+          .and_return([valid_status])
+
+        query.filters.clear
+        query.add_filter('status_id', '=', values)
+
+        query.valid_subset!
+      end
+
+      context 'for a status filter having valid and invalid values' do
+        let(:values) { [valid_status.id.to_s, '99999'] }
+
+        it 'leaves the filter' do
+          expect(query.filters.length).to eq 1
+        end
+
+        it 'leaves only the invalid value' do
+          expect(query.filters[0].values)
+            .to match_array [valid_status.id.to_s]
+        end
+      end
+
+      context 'for a status filter having only invalid values' do
+        let(:values) { ['99999'] }
+
+        it 'removes the filter' do
+          expect(query.filters.length).to eq 0
+        end
+      end
+    end
+
+    context 'group_by' do
+      before do
+        query.group_by = group_by
+      end
+
+      context 'valid' do
+        let(:group_by) { 'project' }
+
+        it 'leaves the value untouched' do
+          query.valid_subset!
+
+          expect(query.group_by).to eql group_by
+        end
+      end
+
+      context 'invalid' do
+        let(:group_by) { 'cf_0815' }
+
+        it 'removes the group by' do
+          query.valid_subset!
+
+          expect(query.group_by).to be_nil
+        end
+      end
+    end
+
+    context 'sort_criteria' do
+      before do
+        query.sort_criteria = sort_by
+      end
+
+      context 'valid' do
+        let(:sort_by) { [['project', 'desc']] }
+
+        it 'leaves the value untouched' do
+          query.valid_subset!
+
+          expect(query.sort_criteria).to eql sort_by
+        end
+      end
+
+      context 'invalid' do
+        let(:sort_by) { [['cf_0815', 'desc']] }
+
+        it 'removes the sorting' do
+          query.valid_subset!
+
+          expect(query.sort_criteria).to be_empty
+        end
+      end
+
+      context 'partially invalid' do
+        let(:sort_by) { [['cf_0815', 'desc'], ['project', 'desc']] }
+
+        it 'removes the offending values from sort' do
+          query.valid_subset!
+
+          expect(query.sort_criteria).to match_array [['project', 'desc']]
+        end
+      end
+    end
+  end
+
   describe '#filter_for' do
     context 'for a status_id filter' do
       before do

spec/support/components/work_packages/filters.rb

@@ -74,7 +74,18 @@ module Components
 
         expect(page).to have_select("operators-#{id}", selected: operator)
 
-        expect_value(id, value)
+        if value
+          expect_value(id, value)
+        else
+          expect(page).to have_no_select("values-#{id}")
+        end
+      end
+
+      def expect_no_filter_by(name, selector = nil)
+        id = selector || name.downcase
+
+        expect(page).to have_no_select("operators-#{id}")
+        expect(page).to have_no_select("values-#{id}")
       end
 
       def remove_filter(field)

spec/support/pages/work_packages_table.rb

@@ -40,6 +40,10 @@ module Pages
       visit "#{path}?query_id=#{query.id}"
     end
 
+    def visit_with_params(params)
+      visit "#{path}?#{params}"
+    end
+
     def expect_work_package_listed(*work_packages)
       within(table_container) do
         work_packages.each do |wp|
@@ -165,6 +169,18 @@ module Pages
       expect_title name
     end
 
+    def save
+      click_setting_item /Save$/
+    end
+
+    def group_by(name)
+      click_setting_item 'Group by ...'
+
+      select name, from: 'Group by'
+
+      click_button 'Apply'
+    end
+
     def open_filter_section
       unless page.has_selector?('#work-packages-filter-toggle-button.-active')
         click_button('work-packages-filter-toggle-button')