From b2754cd26c90dff9c87890d0b00d595d29c4b4f2 Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Mon, 7 Oct 2024 09:54:17 +0200 Subject: [PATCH] move validations concern to filtered_column Co-authored-by: mfo --- .../column_filter_component.html.haml | 2 +- .../instructeurs/procedures_controller.rb | 2 +- app/models/filtered_column.rb | 28 +++++++++ app/models/procedure_presentation.rb | 29 +-------- ...ean_invalid_procedure_presentation_task.rb | 4 +- .../procedures_controller_spec.rb | 2 +- spec/models/filtered_column_spec.rb | 59 +++++++++++++++++++ spec/models/procedure_presentation_spec.rb | 13 +--- ...nvalid_procedure_presentation_task_spec.rb | 4 +- 9 files changed, 100 insertions(+), 43 deletions(-) create mode 100644 spec/models/filtered_column_spec.rb diff --git a/app/components/instructeurs/column_filter_component/column_filter_component.html.haml b/app/components/instructeurs/column_filter_component/column_filter_component.html.haml index 484bf88f5..eaa41be42 100644 --- a/app/components/instructeurs/column_filter_component/column_filter_component.html.haml +++ b/app/components/instructeurs/column_filter_component/column_filter_component.html.haml @@ -12,7 +12,7 @@ - if column_type == :enum = select_tag :filter, options_for_select(options_for_select_of_column), id: 'value', name: "#{prefix}[filter]", class: 'fr-select', data: { no_autosubmit: true } - else - %input#value.fr-input{ type: column_type, name: "#{prefix}[filter]", maxlength: ProcedurePresentation::FILTERS_VALUE_MAX_LENGTH, disabled: column.nil? ? true : false, data: { no_autosubmit: true } } + %input#value.fr-input{ type: column_type, name: "#{prefix}[filter]", maxlength: FilteredColumn::FILTERS_VALUE_MAX_LENGTH, disabled: column.nil? ? true : false, data: { no_autosubmit: true } } = hidden_field_tag :statut, statut = submit_tag t('.add_filter'), class: 'fr-btn fr-btn--secondary fr-mt-2w' diff --git a/app/controllers/instructeurs/procedures_controller.rb b/app/controllers/instructeurs/procedures_controller.rb index 68ed3b7df..8a8f0700a 100644 --- a/app/controllers/instructeurs/procedures_controller.rb +++ b/app/controllers/instructeurs/procedures_controller.rb @@ -150,7 +150,7 @@ module Instructeurs if !procedure_presentation.update(filter_params) # complicated way to display inner error messages flash.alert = procedure_presentation.errors - .flat_map { _1.detail[:value].errors.full_messages } + .flat_map { _1.detail[:value].flat_map { |c| c.errors.full_messages } } end redirect_back(fallback_location: instructeur_procedure_url(procedure)) diff --git a/app/models/filtered_column.rb b/app/models/filtered_column.rb index 8578bbcf8..50579b8b5 100644 --- a/app/models/filtered_column.rb +++ b/app/models/filtered_column.rb @@ -1,8 +1,19 @@ # frozen_string_literal: true class FilteredColumn + include ActiveModel::Validations + + FILTERS_VALUE_MAX_LENGTH = 4048 + # https://www.postgresql.org/docs/current/datatype-numeric.html + PG_INTEGER_MAX_VALUE = 2147483647 + attr_reader :column, :filter + delegate :label, to: :column + + validate :check_filter_max_length + validate :check_filter_max_integer + def initialize(column:, filter:) @column = column @filter = filter @@ -11,4 +22,21 @@ class FilteredColumn def ==(other) other&.column == column && other.filter == filter end + + private + + def check_filter_max_length + if @filter.present? && @filter.length.to_i > FILTERS_VALUE_MAX_LENGTH + errors.add( + :base, + "Le filtre #{label} est trop long (maximum: #{FILTERS_VALUE_MAX_LENGTH} caractères)" + ) + end + end + + def check_filter_max_integer + if @column.column == 'id' && @filter.to_i > PG_INTEGER_MAX_VALUE + errors.add(:base, "Le filtre #{label} n'est pas un numéro de dossier possible") + end + end end diff --git a/app/models/procedure_presentation.rb b/app/models/procedure_presentation.rb index d4d98c1a9..05c7c20dc 100644 --- a/app/models/procedure_presentation.rb +++ b/app/models/procedure_presentation.rb @@ -8,18 +8,11 @@ class ProcedurePresentation < ApplicationRecord SLASH = '/' TYPE_DE_CHAMP = 'type_de_champ' - FILTERS_VALUE_MAX_LENGTH = 4048 - # https://www.postgresql.org/docs/current/datatype-numeric.html - PG_INTEGER_MAX_VALUE = 2147483647 - belongs_to :assign_to, optional: false has_many :exports, dependent: :destroy delegate :procedure, :instructeur, to: :assign_to - validate :check_filters_max_length - validate :check_filters_max_integer - attribute :displayed_columns, :column, array: true attribute :sorted_column, :sorted_column @@ -34,6 +27,9 @@ class ProcedurePresentation < ApplicationRecord attribute :expirant_filters, :filtered_column, array: true attribute :archives_filters, :filtered_column, array: true + validates_associated :a_suivre_filters, :suivis_filters, :traites_filters, + :tous_filters, :supprimes_filters, :expirant_filters, :archives_filters + def filters_for(statut) send(filters_name_for(statut)) end @@ -219,25 +215,6 @@ class ProcedurePresentation < ApplicationRecord .find_by(stable_id: column) end - def check_filters_max_length - filters.values.flatten.each do |filter| - next if !filter.is_a?(Hash) - next if filter['value']&.length.to_i <= FILTERS_VALUE_MAX_LENGTH - - errors.add(:base, "Le filtre #{filter['label']} est trop long (maximum: #{FILTERS_VALUE_MAX_LENGTH} caractères)") - end - end - - def check_filters_max_integer - filters.values.flatten.each do |filter| - next if !filter.is_a?(Hash) - next if filter['column'] != 'id' - next if filter['value']&.to_i&. < PG_INTEGER_MAX_VALUE - - errors.add(:base, "Le filtre #{filter['label']} n'est pas un numéro de dossier possible") - end - end - def self.sanitized_column(association, column) table = if association == 'self' Dossier.table_name diff --git a/app/tasks/maintenance/clean_invalid_procedure_presentation_task.rb b/app/tasks/maintenance/clean_invalid_procedure_presentation_task.rb index 9da038ac4..669c6d3bf 100644 --- a/app/tasks/maintenance/clean_invalid_procedure_presentation_task.rb +++ b/app/tasks/maintenance/clean_invalid_procedure_presentation_task.rb @@ -2,7 +2,7 @@ module Maintenance # PR: 10774 - # why: postgres does not support integer greater than ProcedurePresentation::PG_INTEGER_MAX_VALUE) + # why: postgres does not support integer greater than FilteredColumn::PG_INTEGER_MAX_VALUE) # it occures when user copypaste the dossier id twice (like missed copy paste,paste) # once this huge integer is saved on procedure presentation, page with this filter can't be loaded # when: run this migration when it appears in your maintenance tasks list, this file fix the data and we added some validations too @@ -16,7 +16,7 @@ module Maintenance filters_by_status.reject do |filter| filter.is_a?(Hash) && filter['column'] == 'id' && - (filter['value']&.to_i&. >= ProcedurePresentation::PG_INTEGER_MAX_VALUE) + (filter['value']&.to_i&. >= FilteredColumn::PG_INTEGER_MAX_VALUE) end end element.save diff --git a/spec/controllers/instructeurs/procedures_controller_spec.rb b/spec/controllers/instructeurs/procedures_controller_spec.rb index d5954af14..996966fd8 100644 --- a/spec/controllers/instructeurs/procedures_controller_spec.rb +++ b/spec/controllers/instructeurs/procedures_controller_spec.rb @@ -906,7 +906,7 @@ describe Instructeurs::ProceduresController, type: :controller do subject do column = procedure.find_column(label: "Nom") - post :add_filter, params: { procedure_id: procedure.id, a_suivre_filters: { id: column.id, filter: "n" * 110 } } + post :add_filter, params: { procedure_id: procedure.id, a_suivre_filters: [{ id: column.id, filter: "n" * 4049 }] } end it 'should render the error' do diff --git a/spec/models/filtered_column_spec.rb b/spec/models/filtered_column_spec.rb new file mode 100644 index 000000000..f4882d960 --- /dev/null +++ b/spec/models/filtered_column_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +describe FilteredColumn do + describe '#check_filters_max_length' do + let(:column) { Column.new(procedure_id: 1, table: 'table', column: 'column', label: 'label') } + let(:filtered_column) { described_class.new(column:, filter:) } + + before { filtered_column.valid? } + + context 'when the filter is too long' do + let(:filter) { 'a' * (FilteredColumn::FILTERS_VALUE_MAX_LENGTH + 1) } + + it 'adds an error' do + expect(filtered_column.errors.map(&:message)).to include(/Le filtre label est trop long/) + end + end + + context 'when then filter is not too long' do + let(:filter) { 'a' * FilteredColumn::FILTERS_VALUE_MAX_LENGTH } + + it 'does not add an error' do + expect(filtered_column.errors).to be_empty + end + end + + context 'when the filter is empty' do + let(:filter) { nil } + + it 'does not add an error' do + expect(filtered_column.errors).to be_empty + end + end + end + + describe '#check_filters_max_integer' do + context 'when the target column is an id column' do + let(:column) { Column.new(procedure_id: 1, table: 'table', column: 'id', label: 'label') } + let(:filtered_column) { described_class.new(column:, filter:) } + + before { filtered_column.valid? } + + context 'when the filter is too high' do + let(:filter) { (FilteredColumn::PG_INTEGER_MAX_VALUE + 1).to_s } + + it 'adds an error' do + expect(filtered_column.errors.map(&:message)).to include(/Le filtre label n'est pas un numéro de dossier possible/) + end + end + + context 'when the filter is not too high' do + let(:filter) { FilteredColumn::PG_INTEGER_MAX_VALUE.to_s } + + it 'does not add an error' do + expect(filtered_column.errors).to be_empty + end + end + end + end +end diff --git a/spec/models/procedure_presentation_spec.rb b/spec/models/procedure_presentation_spec.rb index 2fd15c6e2..ecf427de9 100644 --- a/spec/models/procedure_presentation_spec.rb +++ b/spec/models/procedure_presentation_spec.rb @@ -48,16 +48,9 @@ describe ProcedurePresentation do end context 'of filters' do - it do - expect(build(:procedure_presentation, filters: { "suivis" => [{ table: "user", column: "reset_password_token", "order" => "asc" }] })).to be_invalid - expect(build(:procedure_presentation, filters: { "suivis" => [{ table: "user", column: "email", "value" => "exceedingly long filter value" * 1000 }] })).to be_invalid - end - - describe 'check_filters_max_integer' do - it do - expect(build(:procedure_presentation, filters: { "suivis" => [{ table: "self", column: "id", "value" => ProcedurePresentation::PG_INTEGER_MAX_VALUE.to_s }] })).to be_invalid - expect(build(:procedure_presentation, filters: { "suivis" => [{ table: "self", column: "id", "value" => (ProcedurePresentation::PG_INTEGER_MAX_VALUE - 1).to_s }] })).to be_valid - end + it 'validates the filter_column objects' do + expect(build(:procedure_presentation, "suivis_filters": [{ id: { column_id: "user/email", procedure_id: }, "filter": "not so long filter value" }])).to be_valid + expect(build(:procedure_presentation, "suivis_filters": [{ id: { column_id: "user/email", procedure_id: }, "filter": "exceedingly long filter value" * 400 }])).to be_invalid end end end diff --git a/spec/tasks/maintenance/clean_invalid_procedure_presentation_task_spec.rb b/spec/tasks/maintenance/clean_invalid_procedure_presentation_task_spec.rb index 6b95fe523..ba0915b52 100644 --- a/spec/tasks/maintenance/clean_invalid_procedure_presentation_task_spec.rb +++ b/spec/tasks/maintenance/clean_invalid_procedure_presentation_task_spec.rb @@ -14,14 +14,14 @@ module Maintenance before { element.update_column(:filters, filters) } context 'when filter is valid' do - let(:filters) { { "suivis" => [{ 'table' => "self", 'column' => "id", "value" => (ProcedurePresentation::PG_INTEGER_MAX_VALUE - 1).to_s }] } } + let(:filters) { { "suivis" => [{ 'table' => "self", 'column' => "id", "value" => (FilteredColumn::PG_INTEGER_MAX_VALUE - 1).to_s }] } } it 'keeps it filters' do expect { subject }.not_to change { element.reload.filters } end end context 'when filter is invalid, drop it' do - let(:filters) { { "suivis" => [{ 'table' => "self", 'column' => "id", "value" => (ProcedurePresentation::PG_INTEGER_MAX_VALUE).to_s }] } } + let(:filters) { { "suivis" => [{ 'table' => "self", 'column' => "id", "value" => (FilteredColumn::PG_INTEGER_MAX_VALUE).to_s }] } } it 'drop invalid filters' do expect { subject }.to change { element.reload.filters }.to({ "suivis" => [] }) end