From 695426316c49d7b69314292fda482b175a7c130a Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 12 Sep 2018 10:05:59 +0200 Subject: [PATCH 1/4] [#2258] Make linked menus more robust in the face of bad configuration --- .../types_de_champ/linked_drop_down_list_type_de_champ.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/types_de_champ/linked_drop_down_list_type_de_champ.rb b/app/models/types_de_champ/linked_drop_down_list_type_de_champ.rb index c1aa269aa..904e05a40 100644 --- a/app/models/types_de_champ/linked_drop_down_list_type_de_champ.rb +++ b/app/models/types_de_champ/linked_drop_down_list_type_de_champ.rb @@ -27,7 +27,7 @@ class TypesDeChamp::LinkedDropDownListTypeDeChamp < TypesDeChamp::TypeDeChampBas chunked.map do |chunk| primary, *secondary = chunk secondary.unshift('') - [PRIMARY_PATTERN.match(primary)[1], secondary] + [PRIMARY_PATTERN.match(primary)&.[](1), secondary] end end end From 3fea14c07d6bb29a244e066697bb708282d7a09c Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Tue, 11 Sep 2018 18:54:27 +0200 Subject: [PATCH 2/4] [#2258] Let dynamic type validate the type de champ --- app/models/type_de_champ.rb | 11 ++++++++++ .../types_de_champ/type_de_champ_base.rb | 2 ++ spec/models/type_de_champ_shared_example.rb | 21 +++++++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/app/models/type_de_champ.rb b/app/models/type_de_champ.rb index 79aee06b4..33cfd914a 100644 --- a/app/models/type_de_champ.rb +++ b/app/models/type_de_champ.rb @@ -59,6 +59,17 @@ class TypeDeChamp < ApplicationRecord before_validation :check_mandatory before_save :remove_piece_justificative_template, if: -> { type_champ_changed? } + def valid?(context = nil) + super + if dynamic_type.present? + dynamic_type.valid? + errors.merge!(dynamic_type.errors) + end + errors.empty? + end + + alias_method :validate, :valid? + def set_dynamic_type @dynamic_type = type_champ.present? ? self.class.type_champ_to_class_name(type_champ).constantize.new(self) : nil end diff --git a/app/models/types_de_champ/type_de_champ_base.rb b/app/models/types_de_champ/type_de_champ_base.rb index a04f9d862..a7053dc38 100644 --- a/app/models/types_de_champ/type_de_champ_base.rb +++ b/app/models/types_de_champ/type_de_champ_base.rb @@ -1,4 +1,6 @@ class TypesDeChamp::TypeDeChampBase + include ActiveModel::Validations + def initialize(type_de_champ) @type_de_champ = type_de_champ end diff --git a/spec/models/type_de_champ_shared_example.rb b/spec/models/type_de_champ_shared_example.rb index f23cf2156..092527bbc 100644 --- a/spec/models/type_de_champ_shared_example.rb +++ b/spec/models/type_de_champ_shared_example.rb @@ -79,5 +79,26 @@ shared_examples 'type_de_champ_spec' do end end end + + context 'delegate validation to dynamic type' do + subject { build(:type_de_champ_text) } + let(:dynamic_type) do + Class.new(TypesDeChamp::TypeDeChampBase) do + validate :never_valid + + def never_valid + errors.add(:troll, 'always invalid') + end + end.new(subject) + end + + before { subject.instance_variable_set(:@dynamic_type, dynamic_type) } + + it { is_expected.to be_invalid } + it do + subject.validate + expect(subject.errors.full_messages.to_sentence).to eq('Troll always invalid') + end + end end end From b521095010859a2890e3c207ca18acaf84e1dc60 Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 12 Sep 2018 10:32:10 +0200 Subject: [PATCH 3/4] [#2258] Notify administrator of problems saving the types de champs --- app/controllers/admin/types_de_champ_controller.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/types_de_champ_controller.rb b/app/controllers/admin/types_de_champ_controller.rb index 4a13b80e9..b50d4c2c0 100644 --- a/app/controllers/admin/types_de_champ_controller.rb +++ b/app/controllers/admin/types_de_champ_controller.rb @@ -16,9 +16,12 @@ class Admin::TypesDeChampController < AdminController end def update - @procedure.update(TypesDeChampService.create_update_procedure_params params) + if @procedure.update(TypesDeChampService.create_update_procedure_params params) + flash.now.notice = 'Modifications sauvegardées' + else + flash.now.alert = @procedure.errors.full_messages.join(', ') + end create_facade - flash.now.notice = 'Modifications sauvegardées' render 'show', format: :js end From 998754ab739dd6e90d0e5f2a8a321c03832b7d9d Mon Sep 17 00:00:00 2001 From: Frederic Merizen Date: Wed, 12 Sep 2018 15:19:05 +0200 Subject: [PATCH 4/4] [Fix #2258] Validate options for linked dropdown menus --- .../linked_drop_down_list_type_de_champ.rb | 8 ++ .../types_de_champ/type_de_champ_base.rb | 2 + spec/factories/type_de_champ.rb | 2 +- ...inked_drop_down_list_type_de_champ_spec.rb | 76 +++++++++++++++++-- 4 files changed, 82 insertions(+), 6 deletions(-) diff --git a/app/models/types_de_champ/linked_drop_down_list_type_de_champ.rb b/app/models/types_de_champ/linked_drop_down_list_type_de_champ.rb index 904e05a40..a90b7a0f0 100644 --- a/app/models/types_de_champ/linked_drop_down_list_type_de_champ.rb +++ b/app/models/types_de_champ/linked_drop_down_list_type_de_champ.rb @@ -3,6 +3,8 @@ class TypesDeChamp::LinkedDropDownListTypeDeChamp < TypesDeChamp::TypeDeChampBas delegate :drop_down_list, to: :@type_de_champ + validate :check_presence_of_primary_options + def primary_options primary_options = unpack_options.map(&:first) if primary_options.present? @@ -21,6 +23,12 @@ class TypesDeChamp::LinkedDropDownListTypeDeChamp < TypesDeChamp::TypeDeChampBas private + def check_presence_of_primary_options + if !PRIMARY_PATTERN.match?(drop_down_list.options.second) + errors.add(libelle, "doit commencer par une entrée de menu primaire de la forme --texte--") + end + end + def unpack_options _, *options = drop_down_list.options chunked = options.slice_before(PRIMARY_PATTERN) diff --git a/app/models/types_de_champ/type_de_champ_base.rb b/app/models/types_de_champ/type_de_champ_base.rb index a7053dc38..4f77596c3 100644 --- a/app/models/types_de_champ/type_de_champ_base.rb +++ b/app/models/types_de_champ/type_de_champ_base.rb @@ -1,6 +1,8 @@ class TypesDeChamp::TypeDeChampBase include ActiveModel::Validations + delegate :libelle, to: :@type_de_champ + def initialize(type_de_champ) @type_de_champ = type_de_champ end diff --git a/spec/factories/type_de_champ.rb b/spec/factories/type_de_champ.rb index c208f5aac..363b79476 100644 --- a/spec/factories/type_de_champ.rb +++ b/spec/factories/type_de_champ.rb @@ -54,7 +54,7 @@ FactoryBot.define do end factory :type_de_champ_linked_drop_down_list do type_champ { TypeDeChamp.type_champs.fetch(:linked_drop_down_list) } - drop_down_list { create(:drop_down_list) } + drop_down_list { create(:drop_down_list, value: "--primary--\nsecondary\n") } end factory :type_de_champ_pays do type_champ { TypeDeChamp.type_champs.fetch(:pays) } diff --git a/spec/models/types_de_champ/linked_drop_down_list_type_de_champ_spec.rb b/spec/models/types_de_champ/linked_drop_down_list_type_de_champ_spec.rb index f9bc0af6a..b7ee29bdf 100644 --- a/spec/models/types_de_champ/linked_drop_down_list_type_de_champ_spec.rb +++ b/spec/models/types_de_champ/linked_drop_down_list_type_de_champ_spec.rb @@ -1,12 +1,78 @@ require 'spec_helper' describe TypesDeChamp::LinkedDropDownListTypeDeChamp do + let(:drop_down_list) { build(:drop_down_list, value: menu_options) } + let(:type_de_champ) { build(:type_de_champ_linked_drop_down_list, drop_down_list: drop_down_list) } + + subject { type_de_champ.dynamic_type } + + describe 'validation' do + context 'It must start with one primary option' do + context 'valid menu' do + let(:menu_options) do + <<~END_OPTIONS + --Primary 1-- + secondary 1.1 + secondary 1.2 + --Primary 2-- + secondary 2.1 + secondary 2.2 + secondary 2.3 + END_OPTIONS + end + + it { is_expected.to be_valid } + end + + context 'degenerate but valid menu' do + let(:menu_options) do + <<~END_OPTIONS + --Primary 1-- + END_OPTIONS + end + + it { is_expected.to be_valid } + end + + context 'invalid menus' do + shared_examples 'missing primary option' do + it { is_expected.to be_invalid } + it do + subject.validate + expect(subject.errors.full_messages).to eq ["#{subject.libelle} doit commencer par une entrée de menu primaire de la forme --texte--"] + end + end + + context 'no primary option' do + let(:menu_options) do + <<~END_OPTIONS + secondary 1.1 + secondary 1.2 + END_OPTIONS + end + + it_should_behave_like 'missing primary option' + end + + context 'starting with secondary options' do + let(:menu_options) do + <<~END_OPTIONS + secondary 1.1 + secondary 1.2 + --Primary 2-- + secondary 2.1 + secondary 2.2 + secondary 2.3 + END_OPTIONS + end + + it_should_behave_like 'missing primary option' + end + end + end + end + describe '#unpack_options' do - let(:drop_down_list) { build(:drop_down_list, value: menu_options) } - let(:type_de_champ) { build(:type_de_champ_linked_drop_down_list, drop_down_list: drop_down_list) } - - subject { type_de_champ.dynamic_type } - context 'with no options' do let(:menu_options) { '' } it { expect(subject.secondary_options).to eq({}) }