From 54cb65b8a7d65b0e7f104c7cf7151f5992912000 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Thu, 14 Feb 2019 15:03:56 +0100 Subject: [PATCH 01/45] [#3477] Extract helper --- app/models/procedure_presentation.rb | 59 +++++++++++++++++++--------- 1 file changed, 41 insertions(+), 18 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 39ac4dd92..c7c35c0fb 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -114,38 +114,61 @@ class ProcedurePresentation < ApplicationRecord case table when 'self' date = Time.zone.parse(filter['value']).beginning_of_day rescue nil - if date.present? - dossiers.where("#{column} BETWEEN ? AND ?", date, date + 1.day) - else - [] - end + Filter.new( + dossiers + ).where_datetime_matches(column, date) when 'type_de_champ', 'type_de_champ_private' relation = table == 'type_de_champ' ? :champs : :champs_private - dossiers - .includes(relation) - .where("champs.type_de_champ_id = ?", filter['column'].to_i) - .where("champs.value ILIKE ?", "%#{filter['value']}%") + Filter.new( + dossiers + .includes(relation) + .where("champs.type_de_champ_id = ?", filter['column'].to_i) + ).where_ilike('champs.value', filter['value']) when 'etablissement' if filter['column'] == 'entreprise_date_creation' date = filter['value'].to_date rescue nil - dossiers - .includes(table) - .where("#{column} = ?", date) + Filter.new( + dossiers.includes(table) + ).where_equals(column, date) else - dossiers - .includes(table) - .where("#{column} ILIKE ?", "%#{filter['value']}%") + Filter.new( + dossiers + .includes(table) + ).where_ilike(column, filter['value']) end when 'user', 'individual' - dossiers - .includes(table) - .where("#{column} ILIKE ?", "%#{filter['value']}%") + Filter.new( + dossiers + .includes(table) + ).where_ilike(column, filter['value']) end.pluck(:id) end.reduce(:&) end private + class Filter + def initialize(dossiers) + @dossiers = dossiers + end + + def where_datetime_matches(column, date) + if date.present? + @dossiers.where("#{column} BETWEEN ? AND ?", date, date + 1.day) + else + [] + end + end + + def where_ilike(column, value) + @dossiers.where("#{column} ILIKE ?", "%#{value}%") + end + + def where_equals(column, value) + @dossiers.where("#{column} = ?", value) + end + end + def check_allowed_displayed_fields displayed_fields.each do |field| table = field['table'] From 1431de5cc272c8aec79f62a27beb5de293025652 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Thu, 14 Feb 2019 15:34:00 +0100 Subject: [PATCH 02/45] [#3477] Handle multiply-valued filters --- app/models/procedure_presentation.rb | 32 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index c7c35c0fb..9d3e42ab4 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -111,36 +111,37 @@ class ProcedurePresentation < ApplicationRecord filters[statut].map do |filter| table = filter['table'] column = sanitized_column(filter) + values = [filter['value']] case table when 'self' - date = Time.zone.parse(filter['value']).beginning_of_day rescue nil + dates = values.map { |v| Time.zone.parse(v).beginning_of_day rescue nil } Filter.new( dossiers - ).where_datetime_matches(column, date) + ).where_datetime_matches(column, dates) when 'type_de_champ', 'type_de_champ_private' relation = table == 'type_de_champ' ? :champs : :champs_private Filter.new( dossiers .includes(relation) .where("champs.type_de_champ_id = ?", filter['column'].to_i) - ).where_ilike('champs.value', filter['value']) + ).where_ilike('champs.value', values) when 'etablissement' if filter['column'] == 'entreprise_date_creation' - date = filter['value'].to_date rescue nil + dates = values.map { |v| v.to_date rescue nil } Filter.new( dossiers.includes(table) - ).where_equals(column, date) + ).where_equals(column, dates) else Filter.new( dossiers .includes(table) - ).where_ilike(column, filter['value']) + ).where_ilike(column, values) end when 'user', 'individual' Filter.new( dossiers .includes(table) - ).where_ilike(column, filter['value']) + ).where_ilike(column, values) end.pluck(:id) end.reduce(:&) end @@ -152,20 +153,23 @@ class ProcedurePresentation < ApplicationRecord @dossiers = dossiers end - def where_datetime_matches(column, date) - if date.present? - @dossiers.where("#{column} BETWEEN ? AND ?", date, date + 1.day) + def where_datetime_matches(column, dates) + dates = dates.compact.flat_map { |d| [d, d + 1.day] } + if dates.present? + q = Array.new(dates.count / 2, "(#{column} BETWEEN ? AND ?)").join(' OR ') + @dossiers.where(q, *dates) else [] end end - def where_ilike(column, value) - @dossiers.where("#{column} ILIKE ?", "%#{value}%") + def where_ilike(column, values) + q = Array.new(values.count, "(#{column} ILIKE ?)").join(' OR ') + @dossiers.where(q, *(values.map { |value| "%#{value}%" })) end - def where_equals(column, value) - @dossiers.where("#{column} = ?", value) + def where_equals(column, values) + @dossiers.where("#{column} IN (?)", values) end end From 5d01e37d704f70b83d5a3c4d938e47300158bf1e Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 19 Feb 2019 13:10:24 +0100 Subject: [PATCH 03/45] [#3477] Make filters stand out a little more in spec --- spec/models/procedure_presentation_spec.rb | 28 +++++++++++++++------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/spec/models/procedure_presentation_spec.rb b/spec/models/procedure_presentation_spec.rb index 9ed0e79a0..02c7801a6 100644 --- a/spec/models/procedure_presentation_spec.rb +++ b/spec/models/procedure_presentation_spec.rb @@ -377,25 +377,28 @@ describe ProcedurePresentation do context 'for self table' do context 'for created_at column' do + let(:filter) { [{ 'table' => 'self', 'column' => 'created_at', 'value' => '18/9/2018' }] } + let!(:kept_dossier) { create(:dossier, procedure: procedure, created_at: Time.zone.local(2018, 9, 18, 14, 28)) } let!(:discarded_dossier) { create(:dossier, procedure: procedure, created_at: Time.zone.local(2018, 9, 17, 23, 59)) } - let(:filter) { [{ 'table' => 'self', 'column' => 'created_at', 'value' => '18/9/2018' }] } it { is_expected.to contain_exactly(kept_dossier.id) } end context 'for en_construction_at column' do + let(:filter) { [{ 'table' => 'self', 'column' => 'en_construction_at', 'value' => '17/10/2018' }] } + let!(:kept_dossier) { create(:dossier, :en_construction, procedure: procedure, en_construction_at: Time.zone.local(2018, 10, 17)) } let!(:discarded_dossier) { create(:dossier, :en_construction, procedure: procedure, en_construction_at: Time.zone.local(2013, 1, 1)) } - let(:filter) { [{ 'table' => 'self', 'column' => 'en_construction_at', 'value' => '17/10/2018' }] } it { is_expected.to contain_exactly(kept_dossier.id) } end context 'for updated_at column' do + let(:filter) { [{ 'table' => 'self', 'column' => 'updated_at', 'value' => '18/9/2018' }] } + let(:kept_dossier) { create(:dossier, procedure: procedure) } let(:discarded_dossier) { create(:dossier, procedure: procedure) } - let(:filter) { [{ 'table' => 'self', 'column' => 'updated_at', 'value' => '18/9/2018' }] } before do kept_dossier.touch(time: Time.zone.local(2018, 9, 18, 14, 28)) @@ -406,9 +409,10 @@ describe ProcedurePresentation do end context 'ignore time of day' do + let(:filter) { [{ 'table' => 'self', 'column' => 'en_construction_at', 'value' => '17/10/2018 19:30' }] } + let!(:kept_dossier) { create(:dossier, :en_construction, procedure: procedure, en_construction_at: Time.zone.local(2018, 10, 17, 15, 56)) } let!(:discarded_dossier) { create(:dossier, :en_construction, procedure: procedure, en_construction_at: Time.zone.local(2018, 10, 18, 5, 42)) } - let(:filter) { [{ 'table' => 'self', 'column' => 'en_construction_at', 'value' => '17/10/2018 19:30' }] } it { is_expected.to contain_exactly(kept_dossier.id) } end @@ -416,6 +420,7 @@ describe ProcedurePresentation do context 'for a malformed date' do context 'when its a string' do let(:filter) { [{ 'table' => 'self', 'column' => 'updated_at', 'value' => 'malformed date' }] } + it { is_expected.to match([]) } end @@ -428,10 +433,11 @@ describe ProcedurePresentation do end context 'for type_de_champ table' do + let(:filter) { [{ 'table' => 'type_de_champ', 'column' => type_de_champ.id.to_s, 'value' => 'keep' }] } + let(:kept_dossier) { create(:dossier, procedure: procedure) } let(:discarded_dossier) { create(:dossier, procedure: procedure) } let(:type_de_champ) { procedure.types_de_champ.first } - let(:filter) { [{ 'table' => 'type_de_champ', 'column' => type_de_champ.id.to_s, 'value' => 'keep' }] } before do type_de_champ.champ.create(dossier: kept_dossier, value: 'keep me') @@ -442,10 +448,11 @@ describe ProcedurePresentation do end context 'for type_de_champ_private table' do + let(:filter) { [{ 'table' => 'type_de_champ_private', 'column' => type_de_champ_private.id.to_s, 'value' => 'keep' }] } + let(:kept_dossier) { create(:dossier, procedure: procedure) } let(:discarded_dossier) { create(:dossier, procedure: procedure) } let(:type_de_champ_private) { procedure.types_de_champ_private.first } - let(:filter) { [{ 'table' => 'type_de_champ_private', 'column' => type_de_champ_private.id.to_s, 'value' => 'keep' }] } before do type_de_champ_private.champ.create(dossier: kept_dossier, value: 'keep me') @@ -457,9 +464,10 @@ describe ProcedurePresentation do context 'for etablissement table' do context 'for entreprise_date_creation column' do + let(:filter) { [{ 'table' => 'etablissement', 'column' => 'entreprise_date_creation', 'value' => '21/6/2018' }] } + let!(:kept_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, entreprise_date_creation: Time.zone.local(2018, 6, 21))) } let!(:discarded_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, entreprise_date_creation: Time.zone.local(2008, 6, 21))) } - let(:filter) { [{ 'table' => 'etablissement', 'column' => 'entreprise_date_creation', 'value' => '21/6/2018' }] } it { is_expected.to contain_exactly(kept_dossier.id) } end @@ -467,18 +475,20 @@ describe ProcedurePresentation do context 'for code_postal column' do # All columns except entreprise_date_creation work exacly the same, just testing one + let(:filter) { [{ 'table' => 'etablissement', 'column' => 'code_postal', 'value' => '75017' }] } + let!(:kept_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, code_postal: '75017')) } let!(:discarded_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, code_postal: '25000')) } - let(:filter) { [{ 'table' => 'etablissement', 'column' => 'code_postal', 'value' => '75017' }] } it { is_expected.to contain_exactly(kept_dossier.id) } end end context 'for user table' do + let(:filter) { [{ 'table' => 'user', 'column' => 'email', 'value' => 'keepmail' }] } + let!(:kept_dossier) { create(:dossier, procedure: procedure, user: create(:user, email: 'me@keepmail.com')) } let!(:discarded_dossier) { create(:dossier, procedure: procedure, user: create(:user, email: 'me@discard.com')) } - let(:filter) { [{ 'table' => 'user', 'column' => 'email', 'value' => 'keepmail' }] } it { is_expected.to contain_exactly(kept_dossier.id) } end From 70bf6aecf68164d5a3b8088c6f9fa0fff5a07266 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 19 Feb 2019 13:11:00 +0100 Subject: [PATCH 04/45] [#3477] Filter by mutliple values --- app/models/procedure_presentation.rb | 12 +-- spec/models/procedure_presentation_spec.rb | 115 +++++++++++++++++++++ 2 files changed, 121 insertions(+), 6 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 9d3e42ab4..f6b051151 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -108,10 +108,10 @@ class ProcedurePresentation < ApplicationRecord def filtered_ids(dossiers, statut) dossiers.each { |dossier| assert_matching_procedure(dossier) } - filters[statut].map do |filter| - table = filter['table'] - column = sanitized_column(filter) - values = [filter['value']] + filters[statut].group_by { |filter| filter.slice('table', 'column') } .map do |field, filters| + table = field['table'] + column = sanitized_column(field) + values = filters.map { |filter| filter['value'] } case table when 'self' dates = values.map { |v| Time.zone.parse(v).beginning_of_day rescue nil } @@ -123,10 +123,10 @@ class ProcedurePresentation < ApplicationRecord Filter.new( dossiers .includes(relation) - .where("champs.type_de_champ_id = ?", filter['column'].to_i) + .where("champs.type_de_champ_id = ?", field['column'].to_i) ).where_ilike('champs.value', values) when 'etablissement' - if filter['column'] == 'entreprise_date_creation' + if field['column'] == 'entreprise_date_creation' dates = values.map { |v| v.to_date rescue nil } Filter.new( dossiers.includes(table) diff --git a/spec/models/procedure_presentation_spec.rb b/spec/models/procedure_presentation_spec.rb index 02c7801a6..7fbb4753a 100644 --- a/spec/models/procedure_presentation_spec.rb +++ b/spec/models/procedure_presentation_spec.rb @@ -430,6 +430,23 @@ describe ProcedurePresentation do it { is_expected.to match([]) } end end + + context 'with multiple search values' do + let(:filter) do + [ + { 'table' => 'self', 'column' => 'en_construction_at', 'value' => '17/10/2018' }, + { 'table' => 'self', 'column' => 'en_construction_at', 'value' => '19/10/2018' } + ] + end + + let!(:kept_dossier) { create(:dossier, :en_construction, procedure: procedure, en_construction_at: Time.zone.local(2018, 10, 17)) } + let!(:other_kept_dossier) { create(:dossier, :en_construction, procedure: procedure, en_construction_at: Time.zone.local(2018, 10, 19)) } + let!(:discarded_dossier) { create(:dossier, :en_construction, procedure: procedure, en_construction_at: Time.zone.local(2013, 1, 1)) } + + it 'returns every dossier that matches any of the search criteria for a given column' do + is_expected.to contain_exactly(kept_dossier.id, other_kept_dossier.id) + end + end end context 'for type_de_champ table' do @@ -445,6 +462,25 @@ describe ProcedurePresentation do end it { is_expected.to contain_exactly(kept_dossier.id) } + + context 'with multiple search values' do + let(:filter) do + [ + { 'table' => 'type_de_champ', 'column' => type_de_champ.id.to_s, 'value' => 'keep' }, + { 'table' => 'type_de_champ', 'column' => type_de_champ.id.to_s, 'value' => 'and' } + ] + end + + let(:other_kept_dossier) { create(:dossier, procedure: procedure) } + + before do + type_de_champ.champ.create(dossier: other_kept_dossier, value: 'and me too') + end + + it 'returns every dossier that matches any of the search criteria for a given column' do + is_expected.to contain_exactly(kept_dossier.id, other_kept_dossier.id) + end + end end context 'for type_de_champ_private table' do @@ -460,6 +496,25 @@ describe ProcedurePresentation do end it { is_expected.to contain_exactly(kept_dossier.id) } + + context 'with multiple search values' do + let(:filter) do + [ + { 'table' => 'type_de_champ_private', 'column' => type_de_champ_private.id.to_s, 'value' => 'keep' }, + { 'table' => 'type_de_champ_private', 'column' => type_de_champ_private.id.to_s, 'value' => 'and' } + ] + end + + let(:other_kept_dossier) { create(:dossier, procedure: procedure) } + + before do + type_de_champ_private.champ.create(dossier: other_kept_dossier, value: 'and me too') + end + + it 'returns every dossier that matches any of the search criteria for a given column' do + is_expected.to contain_exactly(kept_dossier.id, other_kept_dossier.id) + end + end end context 'for etablissement table' do @@ -470,6 +525,21 @@ describe ProcedurePresentation do let!(:discarded_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, entreprise_date_creation: Time.zone.local(2008, 6, 21))) } it { is_expected.to contain_exactly(kept_dossier.id) } + + context 'with multiple search values' do + let(:filter) do + [ + { 'table' => 'etablissement', 'column' => 'entreprise_date_creation', 'value' => '21/6/2016' }, + { 'table' => 'etablissement', 'column' => 'entreprise_date_creation', 'value' => '21/6/2018' } + ] + end + + let!(:other_kept_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, entreprise_date_creation: Time.zone.local(2016, 6, 21))) } + + it 'returns every dossier that matches any of the search criteria for a given column' do + is_expected.to contain_exactly(kept_dossier.id, other_kept_dossier.id) + end + end end context 'for code_postal column' do @@ -481,6 +551,21 @@ describe ProcedurePresentation do let!(:discarded_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, code_postal: '25000')) } it { is_expected.to contain_exactly(kept_dossier.id) } + + context 'with multiple search values' do + let(:filter) do + [ + { 'table' => 'etablissement', 'column' => 'code_postal', 'value' => '75017' }, + { 'table' => 'etablissement', 'column' => 'code_postal', 'value' => '88100' } + ] + end + + let!(:other_kept_dossier) { create(:dossier, procedure: procedure, etablissement: create(:etablissement, code_postal: '88100')) } + + it 'returns every dossier that matches any of the search criteria for a given column' do + is_expected.to contain_exactly(kept_dossier.id, other_kept_dossier.id) + end + end end end @@ -491,6 +576,21 @@ describe ProcedurePresentation do let!(:discarded_dossier) { create(:dossier, procedure: procedure, user: create(:user, email: 'me@discard.com')) } it { is_expected.to contain_exactly(kept_dossier.id) } + + context 'with multiple search values' do + let(:filter) do + [ + { 'table' => 'user', 'column' => 'email', 'value' => 'keepmail' }, + { 'table' => 'user', 'column' => 'email', 'value' => 'beta.gouv.fr' } + ] + end + + let!(:other_kept_dossier) { create(:dossier, procedure: procedure, user: create(:user, email: 'bazinga@beta.gouv.fr')) } + + it 'returns every dossier that matches any of the search criteria for a given column' do + is_expected.to contain_exactly(kept_dossier.id, other_kept_dossier.id) + end + end end context 'for individual table' do @@ -515,6 +615,21 @@ describe ProcedurePresentation do it { is_expected.to contain_exactly(kept_dossier.id) } end + + context 'with multiple search values' do + let(:filter) do + [ + { 'table' => 'individual', 'column' => 'prenom', 'value' => 'Josephine' }, + { 'table' => 'individual', 'column' => 'prenom', 'value' => 'Romuald' } + ] + end + + let!(:other_kept_dossier) { create(:dossier, procedure: procedure, individual: create(:individual, gender: 'M', prenom: 'Romuald', nom: 'Pistis')) } + + it 'returns every dossier that matches any of the search criteria for a given column' do + is_expected.to contain_exactly(kept_dossier.id, other_kept_dossier.id) + end + end end end end From 5e1dc7059adb97ded55cf3facc2a8acd0ad2c325 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 13 Feb 2019 14:41:31 +0100 Subject: [PATCH 05/45] [#3477] Allow selecting the same filter repeatedly --- app/controllers/new_gestionnaire/procedures_controller.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/app/controllers/new_gestionnaire/procedures_controller.rb b/app/controllers/new_gestionnaire/procedures_controller.rb index 80237e092..8e15b44d2 100644 --- a/app/controllers/new_gestionnaire/procedures_controller.rb +++ b/app/controllers/new_gestionnaire/procedures_controller.rb @@ -238,13 +238,7 @@ module NewGestionnaire end def available_fields_to_filters - current_filters_fields_ids = current_filters.map do |field| - "#{field['table']}/#{field['column']}" - end - - procedure_presentation.fields_for_select.reject do |field| - current_filters_fields_ids.include?(field[1]) - end + procedure_presentation.fields_for_select end def eager_load_displayed_fields From 58158938cb24cf1f327fc1d800a2d0195b59e57e Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 13 Feb 2019 17:50:33 +0100 Subject: [PATCH 06/45] [#3477] Extract helper --- .../new_gestionnaire/procedures_controller.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/new_gestionnaire/procedures_controller.rb b/app/controllers/new_gestionnaire/procedures_controller.rb index 8e15b44d2..708dd6f6d 100644 --- a/app/controllers/new_gestionnaire/procedures_controller.rb +++ b/app/controllers/new_gestionnaire/procedures_controller.rb @@ -112,7 +112,7 @@ module NewGestionnaire procedure_presentation.update(displayed_fields: fields) current_sort = procedure_presentation.sort - if !values.include?("#{current_sort['table']}/#{current_sort['column']}") + if !values.include?(field_id(current_sort)) procedure_presentation.update(sort: Procedure.default_sort) end @@ -194,6 +194,10 @@ module NewGestionnaire private + def field_id(field) + "#{field['table']}/#{field['column']}" + end + def statut @statut ||= (params[:statut].presence || 'a-suivre') end @@ -228,9 +232,7 @@ module NewGestionnaire end def displayed_fields_values - procedure_presentation.displayed_fields.map do |field| - "#{field['table']}/#{field['column']}" - end + procedure_presentation.displayed_fields.map { |field| field_id(field) } end def current_filters From bda41d7574b10110ac0ab1f1ec94949eb49e81db Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 13 Feb 2019 17:53:41 +0100 Subject: [PATCH 07/45] [#3477] Clarify `field_id` --- app/controllers/new_gestionnaire/procedures_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/new_gestionnaire/procedures_controller.rb b/app/controllers/new_gestionnaire/procedures_controller.rb index 708dd6f6d..b3792eade 100644 --- a/app/controllers/new_gestionnaire/procedures_controller.rb +++ b/app/controllers/new_gestionnaire/procedures_controller.rb @@ -195,7 +195,7 @@ module NewGestionnaire private def field_id(field) - "#{field['table']}/#{field['column']}" + field.values_at('table', 'column').join('/') end def statut From 822a3f70988e804171a863656c0bfb7e057028d7 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 13 Feb 2019 18:24:41 +0100 Subject: [PATCH 08/45] [#3477] Extract helper to find field --- .../new_gestionnaire/procedures_controller.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/new_gestionnaire/procedures_controller.rb b/app/controllers/new_gestionnaire/procedures_controller.rb index b3792eade..7c25e6555 100644 --- a/app/controllers/new_gestionnaire/procedures_controller.rb +++ b/app/controllers/new_gestionnaire/procedures_controller.rb @@ -102,11 +102,7 @@ module NewGestionnaire end fields = values.map do |value| - table, column = value.split("/") - - procedure_presentation.fields.find do |field| - field['table'] == table && field['column'] == column - end + find_field(*value.split('/')) end procedure_presentation.update(displayed_fields: fields) @@ -145,7 +141,7 @@ module NewGestionnaire if params[:value].present? filters = procedure_presentation.filters table, column = params[:field].split('/') - label = procedure_presentation.fields.find { |c| c['table'] == table && c['column'] == column }['label'] + label = find_field(table, column)['label'] filters[statut] << { 'label' => label, @@ -194,6 +190,10 @@ module NewGestionnaire private + def find_field(table, column) + procedure_presentation.fields.find { |c| c['table'] == table && c['column'] == column } + end + def field_id(field) field.values_at('table', 'column').join('/') end From ac4750e5e3d5bfd8762b858a9c5f38586ab4ca13 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 13 Feb 2019 18:28:33 +0100 Subject: [PATCH 09/45] [#3477] Simplify removing a filter --- app/controllers/new_gestionnaire/procedures_controller.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/controllers/new_gestionnaire/procedures_controller.rb b/app/controllers/new_gestionnaire/procedures_controller.rb index 7c25e6555..06002ac6e 100644 --- a/app/controllers/new_gestionnaire/procedures_controller.rb +++ b/app/controllers/new_gestionnaire/procedures_controller.rb @@ -158,11 +158,9 @@ module NewGestionnaire def remove_filter filters = procedure_presentation.filters - filter_to_remove = current_filters.find do |filter| - filter['table'] == params[:table] && filter['column'] == params[:column] - end - filters[statut] = filters[statut] - [filter_to_remove] + to_remove = params.values_at(:table, :column) + filters[statut].reject! { |filter| filter.values_at('table', 'column') == to_remove } procedure_presentation.update(filters: filters) From 66fa7ef11d7c189fc2c36371811d72d49ab3a540 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 19 Feb 2019 15:28:12 +0100 Subject: [PATCH 10/45] [#3477] Push up reliance on mutable state --- .../new_gestionnaire/procedures_controller.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/new_gestionnaire/procedures_controller.rb b/app/controllers/new_gestionnaire/procedures_controller.rb index 06002ac6e..cec7571f3 100644 --- a/app/controllers/new_gestionnaire/procedures_controller.rb +++ b/app/controllers/new_gestionnaire/procedures_controller.rb @@ -87,7 +87,7 @@ module NewGestionnaire @dossiers = @dossiers.where(id: filtered_sorted_paginated_ids) - eager_load_displayed_fields + @dossiers = eager_load_displayed_fields(@dossiers) @dossiers = @dossiers.sort_by { |d| filtered_sorted_paginated_ids.index(d.id) } @@ -241,7 +241,7 @@ module NewGestionnaire procedure_presentation.fields_for_select end - def eager_load_displayed_fields + def eager_load_displayed_fields(dossiers) procedure_presentation.displayed_fields .reject { |field| field['table'] == 'self' } .group_by do |field| @@ -250,24 +250,24 @@ module NewGestionnaire else field['table'] end - end.each do |group_key, fields| + end.reduce(dossiers) do |dossiers, (group_key, fields)| case group_key when 'type_de_champ_group' if fields.any? { |field| field['table'] == 'type_de_champ' } - @dossiers = @dossiers.includes(:champs).references(:champs) + dossiers = dossiers.includes(:champs).references(:champs) end if fields.any? { |field| field['table'] == 'type_de_champ_private' } - @dossiers = @dossiers.includes(:champs_private).references(:champs_private) + dossiers = dossiers.includes(:champs_private).references(:champs_private) end where_conditions = fields.map do |field| "champs.type_de_champ_id = #{field['column']}" end.join(" OR ") - @dossiers = @dossiers.where(where_conditions) + dossiers.where(where_conditions) else - @dossiers = @dossiers.includes(fields.first['table']) + dossiers.includes(fields.first['table']) end end end From c81adb80fd8e290cea56e2042452d5e7783a2599 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 19 Feb 2019 15:42:58 +0100 Subject: [PATCH 11/45] [#3477] Better choice of case vs if --- .../new_gestionnaire/procedures_controller.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/new_gestionnaire/procedures_controller.rb b/app/controllers/new_gestionnaire/procedures_controller.rb index cec7571f3..84ea6f9cd 100644 --- a/app/controllers/new_gestionnaire/procedures_controller.rb +++ b/app/controllers/new_gestionnaire/procedures_controller.rb @@ -245,14 +245,16 @@ module NewGestionnaire procedure_presentation.displayed_fields .reject { |field| field['table'] == 'self' } .group_by do |field| - if ['type_de_champ', 'type_de_champ_private'].include?(field['table']) + case field['table'] + when 'type_de_champ', 'type_de_champ_private' 'type_de_champ_group' else field['table'] end end.reduce(dossiers) do |dossiers, (group_key, fields)| - case group_key - when 'type_de_champ_group' + if group_key != 'type_de_champ_group' + dossiers.includes(fields.first['table']) + else if fields.any? { |field| field['table'] == 'type_de_champ' } dossiers = dossiers.includes(:champs).references(:champs) end @@ -266,8 +268,6 @@ module NewGestionnaire end.join(" OR ") dossiers.where(where_conditions) - else - dossiers.includes(fields.first['table']) end end end From 393f1b19716e4792ff32f615d857a734c7323d5d Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 19 Feb 2019 16:57:45 +0100 Subject: [PATCH 12/45] [#3477] Move eager_load_displayed_fields to model --- .../new_gestionnaire/procedures_controller.rb | 33 +------------------ app/models/procedure_presentation.rb | 31 +++++++++++++++++ 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/app/controllers/new_gestionnaire/procedures_controller.rb b/app/controllers/new_gestionnaire/procedures_controller.rb index 84ea6f9cd..98914d21c 100644 --- a/app/controllers/new_gestionnaire/procedures_controller.rb +++ b/app/controllers/new_gestionnaire/procedures_controller.rb @@ -87,7 +87,7 @@ module NewGestionnaire @dossiers = @dossiers.where(id: filtered_sorted_paginated_ids) - @dossiers = eager_load_displayed_fields(@dossiers) + @dossiers = procedure_presentation.eager_load_displayed_fields(@dossiers) @dossiers = @dossiers.sort_by { |d| filtered_sorted_paginated_ids.index(d.id) } @@ -241,37 +241,6 @@ module NewGestionnaire procedure_presentation.fields_for_select end - def eager_load_displayed_fields(dossiers) - procedure_presentation.displayed_fields - .reject { |field| field['table'] == 'self' } - .group_by do |field| - case field['table'] - when 'type_de_champ', 'type_de_champ_private' - 'type_de_champ_group' - else - field['table'] - end - end.reduce(dossiers) do |dossiers, (group_key, fields)| - if group_key != 'type_de_champ_group' - dossiers.includes(fields.first['table']) - else - if fields.any? { |field| field['table'] == 'type_de_champ' } - dossiers = dossiers.includes(:champs).references(:champs) - end - - if fields.any? { |field| field['table'] == 'type_de_champ_private' } - dossiers = dossiers.includes(:champs_private).references(:champs_private) - end - - where_conditions = fields.map do |field| - "champs.type_de_champ_id = #{field['column']}" - end.join(" OR ") - - dossiers.where(where_conditions) - end - end - end - def kaminarize(current_page, total) @dossiers.instance_eval <<-EVAL def current_page diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index f6b051151..e413513a6 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -146,6 +146,37 @@ class ProcedurePresentation < ApplicationRecord end.reduce(:&) end + def eager_load_displayed_fields(dossiers) + displayed_fields + .reject { |field| field['table'] == 'self' } + .group_by do |field| + case field['table'] + when 'type_de_champ', 'type_de_champ_private' + 'type_de_champ_group' + else + field['table'] + end + end.reduce(dossiers) do |dossiers, (group_key, fields)| + if group_key != 'type_de_champ_group' + dossiers.includes(fields.first['table']) + else + if fields.any? { |field| field['table'] == 'type_de_champ' } + dossiers = dossiers.includes(:champs).references(:champs) + end + + if fields.any? { |field| field['table'] == 'type_de_champ_private' } + dossiers = dossiers.includes(:champs_private).references(:champs_private) + end + + where_conditions = fields.map do |field| + "champs.type_de_champ_id = #{field['column']}" + end.join(" OR ") + + dossiers.where(where_conditions) + end + end + end + private class Filter From efa71284fce6465736832f4756efc68f8d865b66 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Mon, 4 Mar 2019 17:01:31 +0100 Subject: [PATCH 13/45] [#3477] Test eager_load_displayed_fields --- spec/models/procedure_presentation_spec.rb | 88 ++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/spec/models/procedure_presentation_spec.rb b/spec/models/procedure_presentation_spec.rb index 7fbb4753a..d6f493144 100644 --- a/spec/models/procedure_presentation_spec.rb +++ b/spec/models/procedure_presentation_spec.rb @@ -632,4 +632,92 @@ describe ProcedurePresentation do end end end + + describe '#eager_load_displayed_fields' do + let(:procedure_presentation) { ProcedurePresentation.create(assign_to: assign_to, displayed_fields: [{ 'table' => table, 'column' => column }]) } + let!(:dossier) { create(:dossier, :en_construction, procedure: procedure) } + let(:displayed_dossier) { procedure_presentation.eager_load_displayed_fields(procedure.dossiers).first } + + context 'for type de champ' do + let(:table) { 'type_de_champ' } + let(:column) { procedure.types_de_champ.first.id } + + it 'preloads the champs relation' do + # Ideally, we would only preload the champs for the matching column + + expect(displayed_dossier.association(:champs)).to be_loaded + expect(displayed_dossier.association(:champs_private)).not_to be_loaded + expect(displayed_dossier.association(:user)).not_to be_loaded + expect(displayed_dossier.association(:individual)).not_to be_loaded + expect(displayed_dossier.association(:etablissement)).not_to be_loaded + end + end + + context 'for type de champ private' do + let(:table) { 'type_de_champ_private' } + let(:column) { procedure.types_de_champ_private.first.id } + + it 'preloads the champs relation' do + # Ideally, we would only preload the champs for the matching column + + expect(displayed_dossier.association(:champs)).not_to be_loaded + expect(displayed_dossier.association(:champs_private)).to be_loaded + expect(displayed_dossier.association(:user)).not_to be_loaded + expect(displayed_dossier.association(:individual)).not_to be_loaded + expect(displayed_dossier.association(:etablissement)).not_to be_loaded + end + end + + context 'for user' do + let(:table) { 'user' } + let(:column) { 'email' } + + it 'preloads the user relation' do + expect(displayed_dossier.association(:champs)).not_to be_loaded + expect(displayed_dossier.association(:champs_private)).not_to be_loaded + expect(displayed_dossier.association(:user)).to be_loaded + expect(displayed_dossier.association(:individual)).not_to be_loaded + expect(displayed_dossier.association(:etablissement)).not_to be_loaded + end + end + + context 'for individual' do + let(:table) { 'individual' } + let(:column) { 'nom' } + + it 'preloads the individual relation' do + expect(displayed_dossier.association(:champs)).not_to be_loaded + expect(displayed_dossier.association(:champs_private)).not_to be_loaded + expect(displayed_dossier.association(:user)).not_to be_loaded + expect(displayed_dossier.association(:individual)).to be_loaded + expect(displayed_dossier.association(:etablissement)).not_to be_loaded + end + end + + context 'for etablissement' do + let(:table) { 'etablissement' } + let(:column) { 'siret' } + + it 'preloads the etablissement relation' do + expect(displayed_dossier.association(:champs)).not_to be_loaded + expect(displayed_dossier.association(:champs_private)).not_to be_loaded + expect(displayed_dossier.association(:user)).not_to be_loaded + expect(displayed_dossier.association(:individual)).not_to be_loaded + expect(displayed_dossier.association(:etablissement)).to be_loaded + end + end + + context 'for self' do + let(:table) { 'self' } + let(:column) { 'created_at' } + + it 'does not preload anything' do + expect(displayed_dossier.association(:champs)).not_to be_loaded + expect(displayed_dossier.association(:champs_private)).not_to be_loaded + expect(displayed_dossier.association(:user)).not_to be_loaded + expect(displayed_dossier.association(:individual)).not_to be_loaded + expect(displayed_dossier.association(:etablissement)).not_to be_loaded + end + end + end end From c53370e14c19f8b611a510e90c35b4923f9c2236 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 19 Feb 2019 17:03:29 +0100 Subject: [PATCH 14/45] [#3477] Reuse existing filtering facility --- app/models/procedure_presentation.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index e413513a6..8b86e6f2d 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -168,11 +168,8 @@ class ProcedurePresentation < ApplicationRecord dossiers = dossiers.includes(:champs_private).references(:champs_private) end - where_conditions = fields.map do |field| - "champs.type_de_champ_id = #{field['column']}" - end.join(" OR ") - - dossiers.where(where_conditions) + Filter.new(dossiers) + .where_equals('champs.type_de_champ_id', fields.pluck('column')) end end end From 71f766c434d8dfec65d8dd66f0816fcae61162eb Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 19 Feb 2019 17:06:06 +0100 Subject: [PATCH 15/45] [#3477] Prefer pluck over map --- app/models/procedure_presentation.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 8b86e6f2d..81ce283e9 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -111,7 +111,7 @@ class ProcedurePresentation < ApplicationRecord filters[statut].group_by { |filter| filter.slice('table', 'column') } .map do |field, filters| table = field['table'] column = sanitized_column(field) - values = filters.map { |filter| filter['value'] } + values = filters.pluck('value') case table when 'self' dates = values.map { |v| Time.zone.parse(v).beginning_of_day rescue nil } @@ -272,7 +272,7 @@ class ProcedurePresentation < ApplicationRecord def valid_columns_for_table(table) @column_whitelist ||= fields .group_by { |field| field['table'] } - .map { |table, fields| [table, Set.new(fields.map { |field| field['column'] })] } + .map { |table, fields| [table, Set.new(fields.pluck('column'))] } .to_h @column_whitelist[table] || [] From 8d8376947da0e4d56f6b9671169cfa790f94181f Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 19 Feb 2019 18:20:12 +0100 Subject: [PATCH 16/45] [Fix #3477] Individually remove values from multi-value filter --- .../new_gestionnaire/procedures_controller.rb | 4 +- .../procedures/show.html.haml | 2 +- config/routes.rb | 2 +- .../procedure_filters_spec.rb | 43 +++++++++++++++++-- 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/app/controllers/new_gestionnaire/procedures_controller.rb b/app/controllers/new_gestionnaire/procedures_controller.rb index 98914d21c..428543ef6 100644 --- a/app/controllers/new_gestionnaire/procedures_controller.rb +++ b/app/controllers/new_gestionnaire/procedures_controller.rb @@ -159,8 +159,8 @@ module NewGestionnaire def remove_filter filters = procedure_presentation.filters - to_remove = params.values_at(:table, :column) - filters[statut].reject! { |filter| filter.values_at('table', 'column') == to_remove } + to_remove = params.values_at(:table, :column, :value) + filters[statut].reject! { |filter| filter.values_at('table', 'column', 'value') == to_remove } procedure_presentation.update(filters: filters) diff --git a/app/views/new_gestionnaire/procedures/show.html.haml b/app/views/new_gestionnaire/procedures/show.html.haml index 11f17fc4f..8b000ec33 100644 --- a/app/views/new_gestionnaire/procedures/show.html.haml +++ b/app/views/new_gestionnaire/procedures/show.html.haml @@ -59,7 +59,7 @@ - @current_filters.each do |filter| %span.filter - = link_to remove_filter_gestionnaire_procedure_path(@procedure, statut: @statut, table: filter['table'], column: filter['column']) do + = link_to remove_filter_gestionnaire_procedure_path(@procedure, statut: @statut, table: filter['table'], column: filter['column'], value: filter['value']) do %img.close-icon{ src: image_url("close.svg") } = "#{filter['label'].truncate(50)} : #{filter['value']}" %table.table.dossiers-table.hoverable diff --git a/config/routes.rb b/config/routes.rb index ba28c63cd..7f27cfb24 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -312,7 +312,7 @@ Rails.application.routes.draw do patch 'update_displayed_fields' get 'update_sort/:table/:column' => 'procedures#update_sort', as: 'update_sort' post 'add_filter' - get 'remove_filter/:statut/:table/:column' => 'procedures#remove_filter', as: 'remove_filter' + get 'remove_filter/:statut/:table/:column/:value' => 'procedures#remove_filter', as: 'remove_filter' get 'download_dossiers' resources :dossiers, only: [:show], param: :dossier_id do diff --git a/spec/features/new_gestionnaire/procedure_filters_spec.rb b/spec/features/new_gestionnaire/procedure_filters_spec.rb index 64f14661b..4ea68fa78 100644 --- a/spec/features/new_gestionnaire/procedure_filters_spec.rb +++ b/spec/features/new_gestionnaire/procedure_filters_spec.rb @@ -7,9 +7,11 @@ feature "procedure filters" do let!(:new_unfollow_dossier) { create(:dossier, procedure: procedure, state: Dossier.states.fetch(:en_instruction)) } let!(:champ) { Champ.find_by(type_de_champ_id: type_de_champ.id, dossier_id: new_unfollow_dossier.id) } let!(:new_unfollow_dossier_2) { create(:dossier, procedure: procedure, state: Dossier.states.fetch(:en_instruction)) } + let!(:champ_2) { Champ.find_by(type_de_champ_id: type_de_champ.id, dossier_id: new_unfollow_dossier_2.id) } before do champ.update(value: "Mon champ rempli") + champ_2.update(value: "Mon autre champ rempli différemment") login_as gestionnaire, scope: :gestionnaire visit gestionnaire_procedure_path(procedure) end @@ -66,7 +68,7 @@ feature "procedure filters" do expect(page).not_to have_link(new_unfollow_dossier_2.user.email) end - remove_filter + remove_filter(champ.value) within ".dossiers-table" do expect(page).to have_link(new_unfollow_dossier.id) @@ -77,8 +79,43 @@ feature "procedure filters" do end end - def remove_filter - find(:xpath, "//span[contains(@class, 'filter')]/a").click + scenario "should be able to add and remove two filters for the same field", js: true do + add_filter(type_de_champ.libelle, champ.value) + add_filter(type_de_champ.libelle, champ_2.value) + + expect(page).to have_content("#{type_de_champ.libelle} : #{champ.value}") + + within ".dossiers-table" do + expect(page).to have_link(new_unfollow_dossier.id, exact: true) + expect(page).to have_link(new_unfollow_dossier.user.email) + + expect(page).to have_link(new_unfollow_dossier_2.id, exact: true) + expect(page).to have_link(new_unfollow_dossier_2.user.email) + end + + remove_filter(champ.value) + + within ".dossiers-table" do + expect(page).not_to have_link(new_unfollow_dossier.id) + expect(page).not_to have_link(new_unfollow_dossier.user.email) + + expect(page).to have_link(new_unfollow_dossier_2.id) + expect(page).to have_link(new_unfollow_dossier_2.user.email) + end + + remove_filter(champ_2.value) + + within ".dossiers-table" do + expect(page).to have_link(new_unfollow_dossier.id) + expect(page).to have_link(new_unfollow_dossier.user.email) + + expect(page).to have_link(new_unfollow_dossier_2.id) + expect(page).to have_link(new_unfollow_dossier_2.user.email) + end + end + + def remove_filter(filter_value) + find(:xpath, "(//span[contains(@class, 'filter')]/a[contains(@href, '#{URI.encode(filter_value)}')])[1]").click end def add_filter(column_name, filter_value) From 43243e73d4aef408fb51c11533c602a223f692f4 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 15:57:42 +0100 Subject: [PATCH 17/45] [#3477] Merge two ordering cases --- app/models/procedure_presentation.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 81ce283e9..7c830c524 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -88,19 +88,14 @@ class ProcedurePresentation < ApplicationRecord return (dossiers.order('dossiers.updated_at asc').ids - dossiers_id_with_notification) + dossiers_id_with_notification end - when 'self' - return dossiers - .order("#{column} #{order}") - .pluck(:id) when 'type_de_champ', 'type_de_champ_private' return dossiers .includes(table == 'type_de_champ' ? :champs : :champs_private) .where("champs.type_de_champ_id = #{sort['column'].to_i}") .order("champs.value #{order}") .pluck(:id) - when 'user', 'individual', 'etablissement' - return dossiers - .includes(table) + when 'self', 'user', 'individual', 'etablissement' + return (table == 'self' ? dossiers : dossiers.includes(table)) .order("#{column} #{order}") .pluck(:id) end From 21128d94b6c1b3042f15e2c73e5cb6c8bcde2b12 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 15:59:46 +0100 Subject: [PATCH 18/45] [#3477] Inline only use of variable --- app/models/procedure_presentation.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 7c830c524..4a9e5935e 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -75,7 +75,6 @@ class ProcedurePresentation < ApplicationRecord def sorted_ids(dossiers, gestionnaire) dossiers.each { |dossier| assert_matching_procedure(dossier) } table = sort['table'] - column = sanitized_column(sort) order = sort['order'] case table @@ -96,7 +95,7 @@ class ProcedurePresentation < ApplicationRecord .pluck(:id) when 'self', 'user', 'individual', 'etablissement' return (table == 'self' ? dossiers : dossiers.includes(table)) - .order("#{column} #{order}") + .order("#{sanitized_column(sort)} #{order}") .pluck(:id) end end From cf2b40f6a0dbc5243d63516c2edd78fb93037f1b Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 16:08:35 +0100 Subject: [PATCH 19/45] [#3477] Extract all variables from sort hash --- app/models/procedure_presentation.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 4a9e5935e..7a3ba2f67 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -74,8 +74,7 @@ class ProcedurePresentation < ApplicationRecord def sorted_ids(dossiers, gestionnaire) dossiers.each { |dossier| assert_matching_procedure(dossier) } - table = sort['table'] - order = sort['order'] + table, column, order = sort.values_at('table', 'column', 'order') case table when 'notifications' @@ -90,7 +89,7 @@ class ProcedurePresentation < ApplicationRecord when 'type_de_champ', 'type_de_champ_private' return dossiers .includes(table == 'type_de_champ' ? :champs : :champs_private) - .where("champs.type_de_champ_id = #{sort['column'].to_i}") + .where("champs.type_de_champ_id = #{column.to_i}") .order("champs.value #{order}") .pluck(:id) when 'self', 'user', 'individual', 'etablissement' From 35d5322a9bf4e16fc84f536b61e34a8a41f4c4a4 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 16:11:02 +0100 Subject: [PATCH 20/45] [#3477] Clarify variable name --- app/models/procedure_presentation.rb | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 7a3ba2f67..0703ba12c 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -103,14 +103,14 @@ class ProcedurePresentation < ApplicationRecord dossiers.each { |dossier| assert_matching_procedure(dossier) } filters[statut].group_by { |filter| filter.slice('table', 'column') } .map do |field, filters| table = field['table'] - column = sanitized_column(field) + table_column = sanitized_column(field) values = filters.pluck('value') case table when 'self' dates = values.map { |v| Time.zone.parse(v).beginning_of_day rescue nil } Filter.new( dossiers - ).where_datetime_matches(column, dates) + ).where_datetime_matches(table_column, dates) when 'type_de_champ', 'type_de_champ_private' relation = table == 'type_de_champ' ? :champs : :champs_private Filter.new( @@ -123,18 +123,18 @@ class ProcedurePresentation < ApplicationRecord dates = values.map { |v| v.to_date rescue nil } Filter.new( dossiers.includes(table) - ).where_equals(column, dates) + ).where_equals(table_column, dates) else Filter.new( dossiers .includes(table) - ).where_ilike(column, values) + ).where_ilike(table_column, values) end when 'user', 'individual' Filter.new( dossiers .includes(table) - ).where_ilike(column, values) + ).where_ilike(table_column, values) end.pluck(:id) end.reduce(:&) end @@ -174,23 +174,23 @@ class ProcedurePresentation < ApplicationRecord @dossiers = dossiers end - def where_datetime_matches(column, dates) + def where_datetime_matches(table_column, dates) dates = dates.compact.flat_map { |d| [d, d + 1.day] } if dates.present? - q = Array.new(dates.count / 2, "(#{column} BETWEEN ? AND ?)").join(' OR ') + q = Array.new(dates.count / 2, "(#{table_column} BETWEEN ? AND ?)").join(' OR ') @dossiers.where(q, *dates) else [] end end - def where_ilike(column, values) - q = Array.new(values.count, "(#{column} ILIKE ?)").join(' OR ') + def where_ilike(table_column, values) + q = Array.new(values.count, "(#{table_column} ILIKE ?)").join(' OR ') @dossiers.where(q, *(values.map { |value| "%#{value}%" })) end - def where_equals(column, values) - @dossiers.where("#{column} IN (?)", values) + def where_equals(table_column, values) + @dossiers.where("#{table_column} IN (?)", values) end end From 55d4dcc17439eb50899f1862424abe9ba8d535b6 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 16:13:22 +0100 Subject: [PATCH 21/45] [#3477] Introduce column variable --- app/models/procedure_presentation.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 0703ba12c..3a5f77422 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -102,7 +102,7 @@ class ProcedurePresentation < ApplicationRecord def filtered_ids(dossiers, statut) dossiers.each { |dossier| assert_matching_procedure(dossier) } filters[statut].group_by { |filter| filter.slice('table', 'column') } .map do |field, filters| - table = field['table'] + table, column = field.values_at('table', 'column') table_column = sanitized_column(field) values = filters.pluck('value') case table @@ -116,10 +116,10 @@ class ProcedurePresentation < ApplicationRecord Filter.new( dossiers .includes(relation) - .where("champs.type_de_champ_id = ?", field['column'].to_i) + .where("champs.type_de_champ_id = ?", column.to_i) ).where_ilike('champs.value', values) when 'etablissement' - if field['column'] == 'entreprise_date_creation' + if column == 'entreprise_date_creation' dates = values.map { |v| v.to_date rescue nil } Filter.new( dossiers.includes(table) From 664956d8c6f9c96f6b16b575a30c126d7d1b5a8c Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 16:27:43 +0100 Subject: [PATCH 22/45] [#3477] Move compact nearer to the nil-generating place --- app/models/procedure_presentation.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 3a5f77422..4fc8954bc 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -107,7 +107,9 @@ class ProcedurePresentation < ApplicationRecord values = filters.pluck('value') case table when 'self' - dates = values.map { |v| Time.zone.parse(v).beginning_of_day rescue nil } + dates = values + .map { |v| Time.zone.parse(v).beginning_of_day rescue nil } + .compact Filter.new( dossiers ).where_datetime_matches(table_column, dates) @@ -175,7 +177,7 @@ class ProcedurePresentation < ApplicationRecord end def where_datetime_matches(table_column, dates) - dates = dates.compact.flat_map { |d| [d, d + 1.day] } + dates = dates.flat_map { |d| [d, d + 1.day] } if dates.present? q = Array.new(dates.count / 2, "(#{table_column} BETWEEN ? AND ?)").join(' OR ') @dossiers.where(q, *dates) From 3c96c2e83db668a7bc2dc1c9a58f3333c9046bf5 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 16:53:31 +0100 Subject: [PATCH 23/45] [#3477] Do not use string interpolation for where_datetime_matches --- app/models/procedure_presentation.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 4fc8954bc..891d83e6d 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -112,7 +112,7 @@ class ProcedurePresentation < ApplicationRecord .compact Filter.new( dossiers - ).where_datetime_matches(table_column, dates) + ).where_datetime_matches(column, dates) when 'type_de_champ', 'type_de_champ_private' relation = table == 'type_de_champ' ? :champs : :champs_private Filter.new( @@ -176,11 +176,11 @@ class ProcedurePresentation < ApplicationRecord @dossiers = dossiers end - def where_datetime_matches(table_column, dates) - dates = dates.flat_map { |d| [d, d + 1.day] } + def where_datetime_matches(column, dates) if dates.present? - q = Array.new(dates.count / 2, "(#{table_column} BETWEEN ? AND ?)").join(' OR ') - @dossiers.where(q, *dates) + dates + .map { |date| @dossiers.where(column => date..(date + 1.day)) } + .reduce(:or) else [] end From 91e6671cfb864443e952627ac407029c4466f029 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 16:57:51 +0100 Subject: [PATCH 24/45] [#3477] Do not use string interpolation for where_equals --- app/models/procedure_presentation.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 891d83e6d..ca37b2d4d 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -125,7 +125,7 @@ class ProcedurePresentation < ApplicationRecord dates = values.map { |v| v.to_date rescue nil } Filter.new( dossiers.includes(table) - ).where_equals(table_column, dates) + ).where_equals(table, column, dates) else Filter.new( dossiers @@ -164,7 +164,7 @@ class ProcedurePresentation < ApplicationRecord end Filter.new(dossiers) - .where_equals('champs.type_de_champ_id', fields.pluck('column')) + .where_equals(:champ, :type_de_champ_id, fields.pluck('column')) end end end @@ -191,8 +191,8 @@ class ProcedurePresentation < ApplicationRecord @dossiers.where(q, *(values.map { |value| "%#{value}%" })) end - def where_equals(table_column, values) - @dossiers.where("#{table_column} IN (?)", values) + def where_equals(table, column, values) + @dossiers.where(table.to_s.pluralize => { column => values }) end end From 1f34d971d17571512362b833757e8850a2311b73 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 17:11:47 +0100 Subject: [PATCH 25/45] [#3477] where_equals is trivial, inline it --- app/models/procedure_presentation.rb | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index ca37b2d4d..bc9dd2035 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -123,9 +123,9 @@ class ProcedurePresentation < ApplicationRecord when 'etablissement' if column == 'entreprise_date_creation' dates = values.map { |v| v.to_date rescue nil } - Filter.new( - dossiers.includes(table) - ).where_equals(table, column, dates) + dossiers + .includes(table) + .where(table.pluralize => { column => dates }) else Filter.new( dossiers @@ -163,8 +163,7 @@ class ProcedurePresentation < ApplicationRecord dossiers = dossiers.includes(:champs_private).references(:champs_private) end - Filter.new(dossiers) - .where_equals(:champ, :type_de_champ_id, fields.pluck('column')) + dossiers.where(champs: { type_de_champ_id: fields.pluck('column') }) end end end @@ -190,10 +189,6 @@ class ProcedurePresentation < ApplicationRecord q = Array.new(values.count, "(#{table_column} ILIKE ?)").join(' OR ') @dossiers.where(q, *(values.map { |value| "%#{value}%" })) end - - def where_equals(table, column, values) - @dossiers.where(table.to_s.pluralize => { column => values }) - end end def check_allowed_displayed_fields From 00ca2e0cbb30bb15cb7a071d40db8c8907f7eff3 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 19:12:05 +0100 Subject: [PATCH 26/45] [#3477] Make sanitized_column dryer --- app/models/procedure_presentation.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index bc9dd2035..2f3d9861b 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -270,10 +270,9 @@ class ProcedurePresentation < ApplicationRecord def sanitized_column(field) table = field['table'] - table = ActiveRecord::Base.connection.quote_column_name((table == 'self' ? 'dossier' : table).pluralize) - column = ActiveRecord::Base.connection.quote_column_name(field['column']) - - table + '.' + column + [(table == 'self' ? 'dossier' : table).pluralize, field['column']] + .map { |name| ActiveRecord::Base.connection.quote_column_name(name) } + .join('.') end def dossier_field_service From 7241e43a7c136d4a0107dc93523f98ca7b1065a7 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 19:15:40 +0100 Subject: [PATCH 27/45] [#3477] Allow calling sanitized_column with individual fields rather than a hash --- app/models/procedure_presentation.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 2f3d9861b..6de06f9f1 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -94,7 +94,7 @@ class ProcedurePresentation < ApplicationRecord .pluck(:id) when 'self', 'user', 'individual', 'etablissement' return (table == 'self' ? dossiers : dossiers.includes(table)) - .order("#{sanitized_column(sort)} #{order}") + .order("#{sanitized_column(table, column)} #{order}") .pluck(:id) end end @@ -103,7 +103,7 @@ class ProcedurePresentation < ApplicationRecord dossiers.each { |dossier| assert_matching_procedure(dossier) } filters[statut].group_by { |filter| filter.slice('table', 'column') } .map do |field, filters| table, column = field.values_at('table', 'column') - table_column = sanitized_column(field) + table_column = sanitized_column(table, column) values = filters.pluck('value') case table when 'self' @@ -268,9 +268,8 @@ class ProcedurePresentation < ApplicationRecord @column_whitelist[table] || [] end - def sanitized_column(field) - table = field['table'] - [(table == 'self' ? 'dossier' : table).pluralize, field['column']] + def sanitized_column(table, column) + [(table == 'self' ? 'dossier' : table).pluralize, column] .map { |name| ActiveRecord::Base.connection.quote_column_name(name) } .join('.') end From f34e65c207d81f7b1f9dbb9fe0c589978b8a1a7b Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 19:17:33 +0100 Subject: [PATCH 28/45] [#3477] Make sanitized_column a class method --- app/models/procedure_presentation.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 6de06f9f1..e9e388590 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -94,7 +94,7 @@ class ProcedurePresentation < ApplicationRecord .pluck(:id) when 'self', 'user', 'individual', 'etablissement' return (table == 'self' ? dossiers : dossiers.includes(table)) - .order("#{sanitized_column(table, column)} #{order}") + .order("#{self.class.sanitized_column(table, column)} #{order}") .pluck(:id) end end @@ -103,7 +103,7 @@ class ProcedurePresentation < ApplicationRecord dossiers.each { |dossier| assert_matching_procedure(dossier) } filters[statut].group_by { |filter| filter.slice('table', 'column') } .map do |field, filters| table, column = field.values_at('table', 'column') - table_column = sanitized_column(table, column) + table_column = self.class.sanitized_column(table, column) values = filters.pluck('value') case table when 'self' @@ -268,7 +268,7 @@ class ProcedurePresentation < ApplicationRecord @column_whitelist[table] || [] end - def sanitized_column(table, column) + def self.sanitized_column(table, column) [(table == 'self' ? 'dossier' : table).pluralize, column] .map { |name| ActiveRecord::Base.connection.quote_column_name(name) } .join('.') From e098779c5e24bfde84b26c31fab89e1e9829cfe9 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 19:18:24 +0100 Subject: [PATCH 29/45] [#3477] Accept symbol arguments to sanitized_column --- app/models/procedure_presentation.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index e9e388590..d6c612327 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -269,7 +269,7 @@ class ProcedurePresentation < ApplicationRecord end def self.sanitized_column(table, column) - [(table == 'self' ? 'dossier' : table).pluralize, column] + [(table == 'self' ? 'dossier' : table.to_s).pluralize, column] .map { |name| ActiveRecord::Base.connection.quote_column_name(name) } .join('.') end From d24fb5d186c144305b56d73756dbbb6ffaf2d092 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 26 Feb 2019 19:21:33 +0100 Subject: [PATCH 30/45] [#3477] Let where_ilike take care of necessary sanitizing --- app/models/procedure_presentation.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index d6c612327..e2e16656a 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -103,7 +103,6 @@ class ProcedurePresentation < ApplicationRecord dossiers.each { |dossier| assert_matching_procedure(dossier) } filters[statut].group_by { |filter| filter.slice('table', 'column') } .map do |field, filters| table, column = field.values_at('table', 'column') - table_column = self.class.sanitized_column(table, column) values = filters.pluck('value') case table when 'self' @@ -119,7 +118,7 @@ class ProcedurePresentation < ApplicationRecord dossiers .includes(relation) .where("champs.type_de_champ_id = ?", column.to_i) - ).where_ilike('champs.value', values) + ).where_ilike(:champ, :value, values) when 'etablissement' if column == 'entreprise_date_creation' dates = values.map { |v| v.to_date rescue nil } @@ -130,13 +129,13 @@ class ProcedurePresentation < ApplicationRecord Filter.new( dossiers .includes(table) - ).where_ilike(table_column, values) + ).where_ilike(table, column, values) end when 'user', 'individual' Filter.new( dossiers .includes(table) - ).where_ilike(table_column, values) + ).where_ilike(table, column, values) end.pluck(:id) end.reduce(:&) end @@ -185,7 +184,8 @@ class ProcedurePresentation < ApplicationRecord end end - def where_ilike(table_column, values) + def where_ilike(table, column, values) + table_column = ProcedurePresentation.sanitized_column(table, column) q = Array.new(values.count, "(#{table_column} ILIKE ?)").join(' OR ') @dossiers.where(q, *(values.map { |value| "%#{value}%" })) end From 9096f923b15e6ba1a048d3c924c065df5ee8d405 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 11:36:04 +0100 Subject: [PATCH 31/45] [#3477] Cleaner way to return no dossiers --- app/models/procedure_presentation.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index e2e16656a..4a9bbb59b 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -180,7 +180,7 @@ class ProcedurePresentation < ApplicationRecord .map { |date| @dossiers.where(column => date..(date + 1.day)) } .reduce(:or) else - [] + @dossiers.none end end From a87e3ac69753faaec7572bb4042857b301ebf740 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 12:03:53 +0100 Subject: [PATCH 32/45] [#3477] Make filtering by datetimes a proper scope --- app/models/concerns/dossier_filtering_concern.rb | 15 +++++++++++++++ app/models/dossier.rb | 2 ++ app/models/procedure_presentation.rb | 14 +------------- 3 files changed, 18 insertions(+), 13 deletions(-) create mode 100644 app/models/concerns/dossier_filtering_concern.rb diff --git a/app/models/concerns/dossier_filtering_concern.rb b/app/models/concerns/dossier_filtering_concern.rb new file mode 100644 index 000000000..8990a60b8 --- /dev/null +++ b/app/models/concerns/dossier_filtering_concern.rb @@ -0,0 +1,15 @@ +module DossierFilteringConcern + extend ActiveSupport::Concern + + included do + scope :filter_by_datetimes, lambda { |column, dates| + if dates.present? + dates + .map { |date| self.where(column => date..(date + 1.day)) } + .reduce(:or) + else + none + end + } + end +end diff --git a/app/models/dossier.rb b/app/models/dossier.rb index 4c5833b30..0b39fe4b2 100644 --- a/app/models/dossier.rb +++ b/app/models/dossier.rb @@ -1,4 +1,6 @@ class Dossier < ApplicationRecord + include DossierFilteringConcern + enum state: { brouillon: 'brouillon', en_construction: 'en_construction', diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 4a9bbb59b..37dbc9ede 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -109,9 +109,7 @@ class ProcedurePresentation < ApplicationRecord dates = values .map { |v| Time.zone.parse(v).beginning_of_day rescue nil } .compact - Filter.new( - dossiers - ).where_datetime_matches(column, dates) + dossiers.filter_by_datetimes(column, dates) when 'type_de_champ', 'type_de_champ_private' relation = table == 'type_de_champ' ? :champs : :champs_private Filter.new( @@ -174,16 +172,6 @@ class ProcedurePresentation < ApplicationRecord @dossiers = dossiers end - def where_datetime_matches(column, dates) - if dates.present? - dates - .map { |date| @dossiers.where(column => date..(date + 1.day)) } - .reduce(:or) - else - @dossiers.none - end - end - def where_ilike(table, column, values) table_column = ProcedurePresentation.sanitized_column(table, column) q = Array.new(values.count, "(#{table_column} ILIKE ?)").join(' OR ') From 0ba3515d41ca1d3a6f728745dd1bef341cc8b306 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 12:12:34 +0100 Subject: [PATCH 33/45] [#3477] Filter by 'column ilike values' as a scope --- .../concerns/dossier_filtering_concern.rb | 6 ++++ app/models/procedure_presentation.rb | 35 ++++++------------- 2 files changed, 16 insertions(+), 25 deletions(-) diff --git a/app/models/concerns/dossier_filtering_concern.rb b/app/models/concerns/dossier_filtering_concern.rb index 8990a60b8..58bcff54b 100644 --- a/app/models/concerns/dossier_filtering_concern.rb +++ b/app/models/concerns/dossier_filtering_concern.rb @@ -11,5 +11,11 @@ module DossierFilteringConcern none end } + + scope :filter_ilike, lambda { |table, column, values| + table_column = ProcedurePresentation.sanitized_column(table, column) + q = Array.new(values.count, "(#{table_column} ILIKE ?)").join(' OR ') + where(q, *(values.map { |value| "%#{value}%" })) + } end end diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 37dbc9ede..1ba45bc68 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -112,11 +112,10 @@ class ProcedurePresentation < ApplicationRecord dossiers.filter_by_datetimes(column, dates) when 'type_de_champ', 'type_de_champ_private' relation = table == 'type_de_champ' ? :champs : :champs_private - Filter.new( - dossiers - .includes(relation) - .where("champs.type_de_champ_id = ?", column.to_i) - ).where_ilike(:champ, :value, values) + dossiers + .includes(relation) + .where("champs.type_de_champ_id = ?", column.to_i) + .filter_ilike(:champ, :value, values) when 'etablissement' if column == 'entreprise_date_creation' dates = values.map { |v| v.to_date rescue nil } @@ -124,16 +123,14 @@ class ProcedurePresentation < ApplicationRecord .includes(table) .where(table.pluralize => { column => dates }) else - Filter.new( - dossiers - .includes(table) - ).where_ilike(table, column, values) - end - when 'user', 'individual' - Filter.new( dossiers .includes(table) - ).where_ilike(table, column, values) + .filter_ilike(table, column, values) + end + when 'user', 'individual' + dossiers + .includes(table) + .filter_ilike(table, column, values) end.pluck(:id) end.reduce(:&) end @@ -167,18 +164,6 @@ class ProcedurePresentation < ApplicationRecord private - class Filter - def initialize(dossiers) - @dossiers = dossiers - end - - def where_ilike(table, column, values) - table_column = ProcedurePresentation.sanitized_column(table, column) - q = Array.new(values.count, "(#{table_column} ILIKE ?)").join(' OR ') - @dossiers.where(q, *(values.map { |value| "%#{value}%" })) - end - end - def check_allowed_displayed_fields displayed_fields.each do |field| table = field['table'] From f0d83b1de81b732387251417aabcf53fcb4f54df Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 12:35:08 +0100 Subject: [PATCH 34/45] [#3477] ignore invalid dates for now --- app/models/procedure_presentation.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 1ba45bc68..6bcdd8429 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -118,7 +118,9 @@ class ProcedurePresentation < ApplicationRecord .filter_ilike(:champ, :value, values) when 'etablissement' if column == 'entreprise_date_creation' - dates = values.map { |v| v.to_date rescue nil } + dates = values + .map { |v| v.to_date rescue nil } + .compact dossiers .includes(table) .where(table.pluralize => { column => dates }) From cf3f2409dd42b229affd1f321b07baac9d39c7ed Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 16:13:22 +0100 Subject: [PATCH 35/45] [#3477] Clarify what the previous code of eager_load_displayed_fields does --- app/models/procedure_presentation.rb | 45 +++++++++++++++------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 6bcdd8429..9330aa83e 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -138,30 +138,33 @@ class ProcedurePresentation < ApplicationRecord end def eager_load_displayed_fields(dossiers) - displayed_fields - .reject { |field| field['table'] == 'self' } - .group_by do |field| - case field['table'] - when 'type_de_champ', 'type_de_champ_private' - 'type_de_champ_group' - else - field['table'] - end - end.reduce(dossiers) do |dossiers, (group_key, fields)| - if group_key != 'type_de_champ_group' - dossiers.includes(fields.first['table']) - else - if fields.any? { |field| field['table'] == 'type_de_champ' } - dossiers = dossiers.includes(:champs).references(:champs) - end + fields_to_eager_load = displayed_fields.reject { |field| field['table'] == 'self' } - if fields.any? { |field| field['table'] == 'type_de_champ_private' } - dossiers = dossiers.includes(:champs_private).references(:champs_private) - end - - dossiers.where(champs: { type_de_champ_id: fields.pluck('column') }) + relations_to_include = fields_to_eager_load + .pluck('table') + .map do |table| + case table + when 'type_de_champ' + :champs + when 'type_de_champ_private' + :champs_private + else + table end end + .uniq + + champ_fields = fields_to_eager_load.select do |field| + ['type_de_champ', 'type_de_champ_private'].include?(field['table']) + end + + if champ_fields.present? + dossiers + .includes(relations_to_include) + .where(champs: { type_de_champ_id: champ_fields.pluck('column') }) + else + dossiers.includes(relations_to_include) + end end private From 4646e1086496177111b792131ed0201369e9f6f4 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 16:27:16 +0100 Subject: [PATCH 36/45] [#3477] The where clause doesn't do what it's supposed to do --- app/models/procedure_presentation.rb | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 9330aa83e..a8ed4f522 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -154,17 +154,7 @@ class ProcedurePresentation < ApplicationRecord end .uniq - champ_fields = fields_to_eager_load.select do |field| - ['type_de_champ', 'type_de_champ_private'].include?(field['table']) - end - - if champ_fields.present? - dossiers - .includes(relations_to_include) - .where(champs: { type_de_champ_id: champ_fields.pluck('column') }) - else - dossiers.includes(relations_to_include) - end + dossiers.includes(relations_to_include) end private From c9ad2995f1d1b98c872f3a76c7d0a3515a695f4c Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 16:28:31 +0100 Subject: [PATCH 37/45] [#3477] Remove extraneous variable --- app/models/procedure_presentation.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index a8ed4f522..d567a6542 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -138,9 +138,8 @@ class ProcedurePresentation < ApplicationRecord end def eager_load_displayed_fields(dossiers) - fields_to_eager_load = displayed_fields.reject { |field| field['table'] == 'self' } - - relations_to_include = fields_to_eager_load + relations_to_include = displayed_fields + .reject { |field| field['table'] == 'self' } .pluck('table') .map do |table| case table From 7d09624bbe8f8f7d0f7bc94b00407b75de0e4477 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 16:30:19 +0100 Subject: [PATCH 38/45] [#3477] We only use the table anyway --- app/models/procedure_presentation.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index d567a6542..f74c90279 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -139,8 +139,8 @@ class ProcedurePresentation < ApplicationRecord def eager_load_displayed_fields(dossiers) relations_to_include = displayed_fields - .reject { |field| field['table'] == 'self' } .pluck('table') + .reject { |table| table == 'self' } .map do |table| case table when 'type_de_champ' From f64ade355f02a13fba0efd6b633b5455b79b1464 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 16:47:45 +0100 Subject: [PATCH 39/45] [#3477] Fix a copy-paste mistake --- app/models/procedure_presentation.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index f74c90279..13c14a557 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -163,7 +163,7 @@ class ProcedurePresentation < ApplicationRecord table = field['table'] column = field['column'] if !valid_column?(table, column) - errors.add(:filters, "#{table}.#{column} n’est pas une colonne permise") + errors.add(:displayed_fields, "#{table}.#{column} n’est pas une colonne permise") end end end From 656f0df3773cc33d04526725fcd3dc4a115c9c46 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Mon, 4 Mar 2019 17:21:48 +0100 Subject: [PATCH 40/45] [#3477] Remove clunky extra method --- app/models/procedure_presentation.rb | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 13c14a557..a41ad8eca 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -171,7 +171,7 @@ class ProcedurePresentation < ApplicationRecord def check_allowed_sort_column table = sort['table'] column = sort['column'] - if !valid_sort_column?(table, column) + if !valid_column?(table, column, EXTRA_SORT_COLUMNS) errors.add(:sort, "#{table}.#{column} n’est pas une colonne permise") end end @@ -222,8 +222,9 @@ class ProcedurePresentation < ApplicationRecord } end - def valid_column?(table, column) - valid_columns_for_table(table).include?(column) + def valid_column?(table, column, extra_columns = {}) + valid_columns_for_table(table).include?(column) || + extra_columns[table]&.include?(column) end def valid_columns_for_table(table) @@ -244,8 +245,4 @@ class ProcedurePresentation < ApplicationRecord def dossier_field_service @dossier_field_service ||= DossierFieldService.new end - - def valid_sort_column?(table, column) - valid_column?(table, column) || EXTRA_SORT_COLUMNS[table]&.include?(column) - end end From e8747f7c381587d2fec4873ecefd4abef018240c Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 16:48:56 +0100 Subject: [PATCH 41/45] [#3477] Extract helper method --- app/models/procedure_presentation.rb | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index a41ad8eca..a3088c964 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -160,20 +160,12 @@ class ProcedurePresentation < ApplicationRecord def check_allowed_displayed_fields displayed_fields.each do |field| - table = field['table'] - column = field['column'] - if !valid_column?(table, column) - errors.add(:displayed_fields, "#{table}.#{column} n’est pas une colonne permise") - end + check_allowed_field(:displayed_fields, field) end end def check_allowed_sort_column - table = sort['table'] - column = sort['column'] - if !valid_column?(table, column, EXTRA_SORT_COLUMNS) - errors.add(:sort, "#{table}.#{column} n’est pas une colonne permise") - end + check_allowed_field(:sort, sort, EXTRA_SORT_COLUMNS) end def check_allowed_sort_order @@ -186,15 +178,19 @@ class ProcedurePresentation < ApplicationRecord def check_allowed_filter_columns filters.each do |_, columns| columns.each do |column| - table = column['table'] - column = column['column'] - if !valid_column?(table, column) - errors.add(:filters, "#{table}.#{column} n’est pas une colonne permise") - end + check_allowed_field(:filters, column) end end end + def check_allowed_field(kind, field, extra_columns = {}) + table = field['table'] + column = field['column'] + if !valid_column?(table, column, extra_columns) + errors.add(kind, "#{table}.#{column} n’est pas une colonne permise") + end + end + def assert_matching_procedure(dossier) if dossier.procedure != procedure raise "Procedure mismatch (expected #{procedure.id}, got #{dossier.procedure.id})" From b4790e50092d8f6e26d1eaf7ce253af53bc4b3c1 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 27 Feb 2019 17:00:41 +0100 Subject: [PATCH 42/45] [#3477] Slightly more compact --- app/models/procedure_presentation.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index a3088c964..75b25f2a9 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -184,8 +184,7 @@ class ProcedurePresentation < ApplicationRecord end def check_allowed_field(kind, field, extra_columns = {}) - table = field['table'] - column = field['column'] + table, column = field.values_at('table', 'column') if !valid_column?(table, column, extra_columns) errors.add(kind, "#{table}.#{column} n’est pas une colonne permise") end From b3c35417253186909e1941a167ad81a71d49b8af Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Thu, 14 Feb 2019 19:02:34 +0100 Subject: [PATCH 43/45] [#3477] Update brakeman config --- config/brakeman.ignore | 73 ++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 46 deletions(-) diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 50bbaa0bc..bc55c5c1e 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -1,85 +1,66 @@ { "ignored_warnings": [ { - "warning_type": "Cross-Site Scripting", - "warning_code": 2, - "fingerprint": "0d61a1267d264f1e61cc2398a2683703ac60878129dc9515542f246a80ad575b", - "check_name": "CrossSiteScripting", - "message": "Unescaped model attribute", - "file": "app/views/champs/carto/show.js.erb", - "line": 5, - "link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting", - "code": "geo_data((Champ.joins(:dossier).where(:dossiers => ({ :user_id => logged_user_ids })).find_by(:id => params.permit(:champ_id)) or CartoChamp.new))", - "render_path": [{"type":"controller","class":"Champs::CartoController","method":"show","line":48,"file":"app/controllers/champs/carto_controller.rb"}], + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "bd1df30f95135357b646e21a03d95498874faffa32e3804fc643e9b6b957ee14", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/concerns/dossier_filtering_concern.rb", + "line": 18, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "where(\"#{values.count} OR #{\"(#{ProcedurePresentation.sanitized_column(table, column)} ILIKE ?)\"}\", *values.map do\n \"%#{value}%\"\n end)", + "render_path": null, "location": { - "type": "template", - "template": "champs/carto/show" + "type": "method", + "class": "DossierFilteringConcern", + "method": null }, - "user_input": "Champ.joins(:dossier).where(:dossiers => ({ :user_id => logged_user_ids }))", - "confidence": "Weak", - "note": "Not an injection because logged_user_ids have no user input" + "user_input": "values.count", + "confidence": "Medium", + "note": "The table and column are escaped, which should make this safe" }, { "warning_type": "SQL Injection", "warning_code": 0, - "fingerprint": "1840f5340630814ea86311e850ebd91b966e6bccd0b6856133528e7745c0695a", + "fingerprint": "e6f09095e3d381bcf6280d2f9b06c239946be3e440330136934f34611bc2b2d9", "check_name": "SQL", "message": "Possible SQL injection", "file": "app/models/procedure_presentation.rb", - "line": 90, + "line": 97, "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "dossiers.order(\"#{sanitized_column(sort)} #{sort[\"order\"]}\")", + "code": "((\"self\" == \"self\") ? (dossiers) : (dossiers.includes(\"self\"))).order(\"#{self.class.sanitized_column(\"self\", column)} #{order}\")", "render_path": null, "location": { "type": "method", "class": "ProcedurePresentation", "method": "sorted_ids" }, - "user_input": "sanitized_column(sort)", + "user_input": "self.class.sanitized_column(\"self\", column)", "confidence": "Weak", - "note": "Not an injection because of `sanitized_column`" + "note": "`table`, `column` and `order` come from the model, which is validated to prevent injection attacks. Furthermore, `table` and `column` are escaped." }, { "warning_type": "SQL Injection", "warning_code": 0, - "fingerprint": "b2feda5e5ae668cdbf0653f134c40bcb9e45499c1b607450e43a0166c4098364", + "fingerprint": "f85ed20c14a223884f624d744ff99070f6fc0697d918f54a08e7786ad70bb243", "check_name": "SQL", "message": "Possible SQL injection", "file": "app/models/procedure_presentation.rb", - "line": 96, + "line": 93, "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "dossiers.includes(((\"type_de_champ\" == \"type_de_champ\") ? (:champs) : (:champs_private))).where(\"champs.type_de_champ_id = #{sort[\"column\"].to_i}\").order(\"champs.value #{sort[\"order\"]}\")", + "code": "dossiers.includes(((\"type_de_champ\" == \"type_de_champ\") ? (:champs) : (:champs_private))).where(\"champs.type_de_champ_id = #{column.to_i}\").order(\"champs.value #{order}\")", "render_path": null, "location": { "type": "method", "class": "ProcedurePresentation", "method": "sorted_ids" }, - "user_input": "sort[\"order\"]", + "user_input": "order", "confidence": "Weak", - "note": "Not an injection because `sort[\"order\"]` has passed `check_allowed_sort_order`" - }, - { - "warning_type": "SQL Injection", - "warning_code": 0, - "fingerprint": "e0e5b55126891df8fe144835ea99367ffd7a92ae6d7227a923fe79f4a79f67f4", - "check_name": "SQL", - "message": "Possible SQL injection", - "file": "app/models/procedure_presentation.rb", - "line": 101, - "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "dossiers.includes(\"user\").order(\"#{sanitized_column(sort)} #{sort[\"order\"]}\")", - "render_path": null, - "location": { - "type": "method", - "class": "ProcedurePresentation", - "method": "sorted_ids" - }, - "user_input": "sanitized_column(sort)", - "confidence": "Weak", - "note": "Not an injection because of `sanitized_column`" + "note": "`column` and `order` come from the model, which is validated to prevent injection attacks. Furthermore, the sql injection attack on `column` would need to survive the `to_i`" } ], - "updated": "2018-10-16 11:28:34 +0300", + "updated": "2019-03-04 11:59:49 +0100", "brakeman_version": "4.3.1" } From c43cde56473f7034f8f09741c39f57e543dba255 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Mon, 4 Mar 2019 18:07:47 +0100 Subject: [PATCH 44/45] [#3477] Show and / or relationship between filters --- .../new_gestionnaire/procedures/show.html.haml | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/app/views/new_gestionnaire/procedures/show.html.haml b/app/views/new_gestionnaire/procedures/show.html.haml index 8b000ec33..003929fd6 100644 --- a/app/views/new_gestionnaire/procedures/show.html.haml +++ b/app/views/new_gestionnaire/procedures/show.html.haml @@ -57,11 +57,16 @@ %br = submit_tag "Ajouter le filtre", class: 'button' - - @current_filters.each do |filter| - %span.filter - = link_to remove_filter_gestionnaire_procedure_path(@procedure, statut: @statut, table: filter['table'], column: filter['column'], value: filter['value']) do - %img.close-icon{ src: image_url("close.svg") } - = "#{filter['label'].truncate(50)} : #{filter['value']}" + - @current_filters.group_by { |filter| filter['table'] }.each_with_index do |(table, filters), i| + - if i > 0 + et + - filters.each_with_index do |filter, i| + - if i > 0 + ou + %span.filter + = link_to remove_filter_gestionnaire_procedure_path(@procedure, statut: @statut, table: filter['table'], column: filter['column'], value: filter['value']) do + %img.close-icon{ src: image_url("close.svg") } + = "#{filter['label'].truncate(50)} : #{filter['value']}" %table.table.dossiers-table.hoverable %thead %tr From 02f35e655de2b3a62a7939f761fcb7245e5693fc Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Mon, 4 Mar 2019 18:41:14 +0100 Subject: [PATCH 45/45] [#3477] Minor simplification --- app/models/procedure_presentation.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index 75b25f2a9..89ee71721 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -101,8 +101,7 @@ class ProcedurePresentation < ApplicationRecord def filtered_ids(dossiers, statut) dossiers.each { |dossier| assert_matching_procedure(dossier) } - filters[statut].group_by { |filter| filter.slice('table', 'column') } .map do |field, filters| - table, column = field.values_at('table', 'column') + filters[statut].group_by { |filter| filter.values_at('table', 'column') } .map do |(table, column), filters| values = filters.pluck('value') case table when 'self'