From 296d19163c8cbe68baf4d08f905ce4ee326324a4 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 23 Sep 2020 15:32:23 +0200 Subject: [PATCH] always use list cf subquery on list custom field filtering The check in place before would check if any custom value, when casted to int would not be greater than zero. This also applies to nil -> 0. This then lead to the non list cf subquery being used when filtering. As a lot of custom values have nil for their value attribute, filtering for list custom fields was only possible in very rare cases. --- .../models/cost_query/custom_field_mixin.rb | 16 +-- .../spec/features/custom_fields_spec.rb | 117 +++++++++--------- 2 files changed, 65 insertions(+), 68 deletions(-) diff --git a/modules/reporting/app/models/cost_query/custom_field_mixin.rb b/modules/reporting/app/models/cost_query/custom_field_mixin.rb index be9c5dd462..7cce3fe01a 100644 --- a/modules/reporting/app/models/cost_query/custom_field_mixin.rb +++ b/modules/reporting/app/models/cost_query/custom_field_mixin.rb @@ -98,7 +98,7 @@ module CostQuery::CustomFieldMixin @class_name = class_name dont_inherit :group_fields db_field table_name - if field.list? && all_values_int?(field) + if field.list? join_table list_join_table(field) else join_table default_join_table(field) @@ -107,15 +107,6 @@ module CostQuery::CustomFieldMixin self end - ## - # HACK: CustomValues of lists MAY have non-integer values when the list - # contained invalid values. - def all_values_int?(field) - field.custom_values.pluck(:value).all? { |val| val.to_i > 0 } - rescue StandardError - false - end - def list_join_table(field) cast_as = SQL_TYPES[field.field_format] cf_name = "custom_field#{field.id}" @@ -123,6 +114,9 @@ module CostQuery::CustomFieldMixin custom_values_table = CustomValue.table_name custom_options_table = CustomOption.table_name + # CustomValues of lists MAY have non-integer values when the list contained invalid values. + # Because of this, we do not cast the cv.value but rather the co.id + <<-SQL -- BEGIN Custom Field Join: #{cf_name} LEFT OUTER JOIN ( @@ -133,7 +127,7 @@ module CostQuery::CustomFieldMixin cv.customized_id FROM #{custom_values_table} cv INNER JOIN #{custom_options_table} co - ON cv.custom_field_id = co.custom_field_id AND CAST(cv.value AS decimal(60,3)) = co.id + ON cv.custom_field_id = co.custom_field_id AND cv.value = co.id::VARCHAR ) AS #{cf_name} ON #{cf_name}.customized_type = 'WorkPackage' diff --git a/modules/reporting/spec/features/custom_fields_spec.rb b/modules/reporting/spec/features/custom_fields_spec.rb index ea6bba64c1..c35683b44b 100644 --- a/modules/reporting/spec/features/custom_fields_spec.rb +++ b/modules/reporting/spec/features/custom_fields_spec.rb @@ -34,27 +34,28 @@ describe 'Custom fields reporting', type: :feature, js: true do let(:user) { FactoryBot.create :admin } - let(:work_package) { + let(:work_package) do FactoryBot.create :work_package, - project: project, - custom_values: initial_custom_values - } + project: project, + type: type, + custom_values: initial_custom_values + end - let!(:time_entry1) { + let!(:time_entry1) do FactoryBot.create :time_entry, - user: user, - work_package: work_package, - project: project, - hours: 10 - } + user: user, + work_package: work_package, + project: project, + hours: 10 + end - let!(:time_entry2) { + let!(:time_entry2) do FactoryBot.create :time_entry, - user: user, - work_package: work_package, - project: project, - hours: 2.50 - } + user: user, + work_package: work_package, + project: project, + hours: 2.50 + end def custom_value_for(cf, str) @@ -63,19 +64,25 @@ describe 'Custom fields reporting', type: :feature, js: true do context 'with multi value cf' do let!(:custom_field) do - FactoryBot.create( - :list_wp_custom_field, - name: "List CF", - multi_value: true, - types: [type], - projects: [project], - possible_values: ['First option', 'Second option'] - ) + FactoryBot.create(:list_wp_custom_field, + name: "List CF", + multi_value: true, + types: [type], + projects: [project], + possible_values: ['First option', 'Second option']) end let(:initial_custom_values) { { custom_field.id => custom_value_for(custom_field, 'First option') } } let(:cf_id) { "custom_field#{custom_field.id}" } + # Have a second work package in the test that will have no values + # as this caused problems with casting the nil value of the custom value to 0. + let!(:work_package2) do + FactoryBot.create :work_package, + project: project, + type: type + end + before do login_as(user) visit '/cost_reports' @@ -85,31 +92,31 @@ describe 'Custom fields reporting', type: :feature, js: true do expect(page).to have_selector('#add_filter_select option', text: 'List CF') select 'List CF', from: 'add_filter_select' - # Adds filter to page + # Adds filter to page, filtering out the time entries on the work package expect(page).to have_selector("label##{cf_id}") custom_field_selector = "##{cf_id}_arg_1_val" select = find(custom_field_selector) expect(select).to have_selector('option', text: 'First option') expect(select).to have_selector('option', text: 'Second option') - select.find('option', text: 'First option').select_option + select.find('option', text: 'Second option').select_option find('#query-icon-apply-button').click - # Expect row of work package + # Expect empty result table within('#result-table') do - expect(page).to have_selector('.top.result', text: '12.50 hours') + expect(page).to have_no_selector('.top.result', text: '12.50 hours') end + expect(page).to have_selector('.generic-table--no-results-title') - # Update filter to other value + # Update filter to value the work package has select = find(custom_field_selector) - select.find('option', text: 'Second option').select_option + select.find('option', text: 'First option').select_option find('#query-icon-apply-button').click - # Expect empty result table + # Expect row of work package within('#result-table') do - expect(page).to have_no_selector('.top.result', text: '12.50 hours') + expect(page).to have_selector('.top.result', text: '12.50 hours') end - expect(page).to have_selector('.generic-table--no-results-title') end it 'groups by the multi CF (Regression #26050)' do @@ -141,29 +148,27 @@ describe 'Custom fields reporting', type: :feature, js: true do context 'with additional WP with invalid value' do let!(:custom_field_2) do - FactoryBot.create( - :list_wp_custom_field, - name: "Invalid List CF", - multi_value: true, - types: [type], - projects: [project], - possible_values: ['A', 'B'] - ) + FactoryBot.create(:list_wp_custom_field, + name: "Invalid List CF", + multi_value: true, + types: [type], + projects: [project], + possible_values: %w[A B]) end - let!(:work_package2) { + let!(:work_package2) do FactoryBot.create :work_package, - project: project, - custom_values: { custom_field_2.id => custom_value_for(custom_field_2, 'A')} - } + project: project, + custom_values: { custom_field_2.id => custom_value_for(custom_field_2, 'A')} + end - let!(:time_entry1) { + let!(:time_entry1) do FactoryBot.create :time_entry, - user: user, - work_package: work_package2, - project: project, - hours: 10 - } + user: user, + work_package: work_package2, + project: project, + hours: 10 + end before do CustomValue.find_by(customized_id: work_package2.id).update_columns(value: 'invalid') @@ -196,12 +201,10 @@ describe 'Custom fields reporting', type: :feature, js: true do context 'with text CF' do let(:custom_field) do - FactoryBot.create( - :text_wp_custom_field, - name: 'Text CF', - types: [type], - projects: [project] - ) + FactoryBot.create(:text_wp_custom_field, + name: 'Text CF', + types: [type], + projects: [project]) end let(:initial_custom_values) { { custom_field.id => 'foo' } }