From 725521c3a1687b211c2a53925671b1db224db252 Mon Sep 17 00:00:00 2001 From: Martin Date: Fri, 26 Nov 2021 14:31:30 +0100 Subject: [PATCH 1/9] fix(expiration_banner): only show expirations info when flip is enabled fix(lint): lint haml fix(spec): enable flipper and allow procedure to receive flipper check when checking banner presence fix(doc): add missing documentation on readme regarding system testing with a visual feedback fix(typo): add missing accent clean(PR): feedback from Tchak, better to wrap feature check for expirability by procedure within dossier.expirable? helper --- README.md | 4 ++ app/models/dossier.rb | 2 +- .../dossiers/_expiration_banner.html.haml | 37 ++++++---- app/views/shared/dossiers/_header.html.haml | 2 +- .../dossiers/_short_expires_message.html.haml | 9 --- .../users/dossiers/show/_header.html.haml | 2 +- spec/system/users/brouillon_spec.rb | 2 + .../_expiration_banner.html.haml_spec.rb | 68 +++++++++++++++++++ .../shared/dossiers/_header.html.haml_spec.rb | 56 --------------- 9 files changed, 101 insertions(+), 81 deletions(-) delete mode 100644 app/views/shared/dossiers/_short_expires_message.html.haml create mode 100644 spec/views/shared/dossiers/_expiration_banner.html.haml_spec.rb delete mode 100644 spec/views/shared/dossiers/_header.html.haml_spec.rb diff --git a/README.md b/README.md index 73acaae72..226ab9857 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,10 @@ Pour exécuter les tests de l'application, plusieurs possibilités : bin/rspec --only-failures +- Lancer un ou des tests systèmes avec un browser + + NO_HEADLESS=1 bin/rspec spec/system + ### Ajout de taches à exécuter au déploiement rails generate after_party:task task_name diff --git a/app/models/dossier.rb b/app/models/dossier.rb index aebc6a385..08d12bf3d 100644 --- a/app/models/dossier.rb +++ b/app/models/dossier.rb @@ -545,7 +545,7 @@ class Dossier < ApplicationRecord end def expirable? - [brouillon?, en_construction?, termine?].any? + [brouillon?, en_construction?, termine? && procedure.feature_enabled?(:procedure_process_expired_dossiers_termine)].any? end def approximative_expiration_date_reference diff --git a/app/views/shared/dossiers/_expiration_banner.html.haml b/app/views/shared/dossiers/_expiration_banner.html.haml index 5f55b4be9..8f4689117 100644 --- a/app/views/shared/dossiers/_expiration_banner.html.haml +++ b/app/views/shared/dossiers/_expiration_banner.html.haml @@ -1,14 +1,25 @@ -- if dossier.expirable? && dossier.close_to_expiration? - .card.warning.mt-2.mb-3 - .card-title= t('shared.dossiers.header.banner.title') - %p - - if dossier.brouillon? - = t('shared.dossiers.header.banner.states.brouillon') - - elsif dossier.en_construction? - = t('shared.dossiers.header.banner.states.en_construction') - - elsif dossier.termine? - = t('shared.dossiers.header.banner.states.termine') +-# small expires mention +- if dossier.expirable? + %p.expires_at + %small= t("shared.dossiers.header.expires_at.#{dossier.state}", date: safe_expiration_date(dossier)) + -# big banner warning + - if dossier.close_to_expiration? + .card.warning.mt-2.mb-3 + .card-title= t('shared.dossiers.header.banner.title') + %p + - if dossier.brouillon? + = t('shared.dossiers.header.banner.states.brouillon') + - elsif dossier.en_construction? + = t('shared.dossiers.header.banner.states.en_construction') + - elsif dossier.termine? + = t('shared.dossiers.header.banner.states.termine') + + - if dossier.expiration_can_be_extended? + %br + = button_to t('shared.dossiers.header.banner.button_delay_expiration'), users_dossier_repousser_expiration_path(dossier), class: 'button secondary mt-2' + + +- else + %p.expires_at_en_instruction + %small= t("shared.dossiers.header.expires_at.en_instruction") - - if dossier.expiration_can_be_extended? - %br - = button_to t('shared.dossiers.header.banner.button_delay_expiration'), users_dossier_repousser_expiration_path(dossier), class: 'button secondary mt-2' diff --git a/app/views/shared/dossiers/_header.html.haml b/app/views/shared/dossiers/_header.html.haml index 056dceff1..c34c17f10 100644 --- a/app/views/shared/dossiers/_header.html.haml +++ b/app/views/shared/dossiers/_header.html.haml @@ -6,7 +6,7 @@ = t('views.users.dossiers.show.header.dossier_number', dossier_id: dossier.id) = t('views.users.dossiers.show.header.created_date', date_du_dossier: I18n.l(dossier.created_at)) - = render(partial: 'shared/dossiers/short_expires_message', locals: {dossier: dossier}) + = render(partial: 'shared/dossiers/expiration_banner', locals: {dossier: dossier}) .header-actions - if current_user.owns?(dossier) diff --git a/app/views/shared/dossiers/_short_expires_message.html.haml b/app/views/shared/dossiers/_short_expires_message.html.haml deleted file mode 100644 index 466c3f880..000000000 --- a/app/views/shared/dossiers/_short_expires_message.html.haml +++ /dev/null @@ -1,9 +0,0 @@ -- if dossier.expirable? - %p.expires_at - %small= t("shared.dossiers.header.expires_at.#{dossier.state}", date: safe_expiration_date(dossier)) -- else - %p.expires_at_en_instruction - %small= t("shared.dossiers.header.expires_at.en_instruction") - - -= render(partial: 'shared/dossiers/expiration_banner', locals: {dossier: dossier}) diff --git a/app/views/users/dossiers/show/_header.html.haml b/app/views/users/dossiers/show/_header.html.haml index 2985d024f..31f4bf110 100644 --- a/app/views/users/dossiers/show/_header.html.haml +++ b/app/views/users/dossiers/show/_header.html.haml @@ -10,7 +10,7 @@ - if dossier.en_construction_at.present? = t('views.users.dossiers.show.header.submit_date', date_du_dossier: I18n.l(dossier.en_construction_at)) - = render(partial: 'shared/dossiers/short_expires_message', locals: {dossier: dossier}) + = render(partial: 'shared/dossiers/expiration_banner', locals: {dossier: dossier}) - if current_user.owns?(dossier) diff --git a/spec/system/users/brouillon_spec.rb b/spec/system/users/brouillon_spec.rb index bc4c3f1b3..e0d6402f4 100644 --- a/spec/system/users/brouillon_spec.rb +++ b/spec/system/users/brouillon_spec.rb @@ -166,6 +166,8 @@ describe 'The user' do end scenario 'extends dossier experation date more than one time, ', js: true do + Flipper.enable(:procedure_process_expired_dossiers_termine) + allow(simple_procedure).to receive(:feature_enabled?).with(:procedure_process_expired_dossiers_termine).and_return(true) user_old_dossier = create(:dossier, procedure: simple_procedure, created_at: simple_procedure.duree_conservation_dossiers_dans_ds.month.ago, diff --git a/spec/views/shared/dossiers/_expiration_banner.html.haml_spec.rb b/spec/views/shared/dossiers/_expiration_banner.html.haml_spec.rb new file mode 100644 index 000000000..2f758db4e --- /dev/null +++ b/spec/views/shared/dossiers/_expiration_banner.html.haml_spec.rb @@ -0,0 +1,68 @@ +describe 'shared/dossiers/expiration_banner.html.haml', type: :view do + include DossierHelper + let(:dossier) { build(:dossier, state, attributes.merge(id: 1, state: state)) } + let(:i18n_key_state) { state } + subject do + render('shared/dossiers/expiration_banner.html.haml', + dossier: dossier, + current_user: build(:user)) + end + + context 'with procedure having procedure_process_expired_dossiers_termine not enabled' do + before { allow(dossier.procedure).to receive(:feature_enabled?).with(:procedure_process_expired_dossiers_termine).and_return(false) } + let(:attributes) { { processed_at: 6.months.ago } } + let(:state) { :accepte } + + it 'render estimated expiration date' do + expect(subject).not_to have_selector('.expires_at') + end + end + + context 'with procedure having procedure_process_expired_dossiers_termine enabled' do + before { allow(dossier.procedure).to receive(:feature_enabled?).with(:procedure_process_expired_dossiers_termine).and_return(true) } + + context 'with dossier.brouillon?' do + let(:attributes) { { created_at: 6.months.ago } } + let(:state) { :brouillon } + + it 'render estimated expiration date' do + expect(subject).to have_selector('.expires_at', + text: I18n.t("shared.dossiers.header.expires_at.#{i18n_key_state}", + date: safe_expiration_date(dossier))) + end + end + + context 'with dossier.en_construction?' do + let(:attributes) { { en_construction_at: 6.months.ago } } + let(:state) { :en_construction } + + it 'render estimated expiration date' do + expect(subject).to have_selector('.expires_at', + text: I18n.t("shared.dossiers.header.expires_at.#{i18n_key_state}", + date: safe_expiration_date(dossier))) + end + end + + context 'with dossier.en_instruction?' do + let(:state) { :en_instruction } + let(:attributes) { {} } + + it 'render estimated expiration date' do + expect(subject).to have_selector('p.expires_at_en_instruction', + text: I18n.t("shared.dossiers.header.expires_at.#{i18n_key_state}")) + end + end + + context 'with dossier.en_processed_at?' do + let(:state) { :accepte } + let(:attributes) { {} } + + it 'render estimated expiration date' do + allow(dossier).to receive(:processed_at).and_return(6.months.ago) + expect(subject).to have_selector('.expires_at', + text: I18n.t("shared.dossiers.header.expires_at.#{i18n_key_state}", + date: safe_expiration_date(dossier))) + end + end + end +end diff --git a/spec/views/shared/dossiers/_header.html.haml_spec.rb b/spec/views/shared/dossiers/_header.html.haml_spec.rb deleted file mode 100644 index 25567efea..000000000 --- a/spec/views/shared/dossiers/_header.html.haml_spec.rb +++ /dev/null @@ -1,56 +0,0 @@ -describe 'shared/dossiers/short_expires_message.html.haml', type: :view do - include DossierHelper - let(:dossier) do - build(:dossier, state, attributes.merge(id: 1, state: state)) - end - let(:i18n_key_state) { state } - subject do - render('shared/dossiers/short_expires_message.html.haml', - dossier: dossier, - current_user: build(:user)) - end - - context 'with dossier.brouillon?' do - let(:attributes) { { created_at: 6.months.ago } } - let(:state) { :brouillon } - - it 'render estimated expiration date' do - expect(subject).to have_selector('.expires_at', - text: I18n.t("shared.dossiers.header.expires_at.#{i18n_key_state}", - date: safe_expiration_date(dossier))) - end - end - - context 'with dossier.en_construction?' do - let(:attributes) { { en_construction_at: 6.months.ago } } - let(:state) { :en_construction } - - it 'render estimated expiration date' do - expect(subject).to have_selector('.expires_at', - text: I18n.t("shared.dossiers.header.expires_at.#{i18n_key_state}", - date: safe_expiration_date(dossier))) - end - end - - context 'with dossier.en_instruction?' do - let(:state) { :en_instruction } - let(:attributes) { {} } - - it 'render estimated expiration date' do - expect(subject).to have_selector('p.expires_at_en_instruction', - text: I18n.t("shared.dossiers.header.expires_at.#{i18n_key_state}")) - end - end - - context 'with dossier.en_processed_at?' do - let(:state) { :accepte } - let(:attributes) { {} } - - it 'render estimated expiration date' do - allow(dossier).to receive(:processed_at).and_return(6.months.ago) - expect(subject).to have_selector('.expires_at', - text: I18n.t("shared.dossiers.header.expires_at.#{i18n_key_state}", - date: safe_expiration_date(dossier))) - end - end -end From 8fc14faa2a2be4de3363b5356e80229fa3ad76fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Vantomme?= Date: Wed, 1 Dec 2021 16:46:11 +0100 Subject: [PATCH 2/9] fix(stat): enforce time travel when dealing with relative dates --- spec/models/stat_spec.rb | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/spec/models/stat_spec.rb b/spec/models/stat_spec.rb index b03ae88c7..b49693ec7 100644 --- a/spec/models/stat_spec.rb +++ b/spec/models/stat_spec.rb @@ -1,3 +1,5 @@ +include ActiveSupport::Testing::TimeHelpers + describe Stat do describe '.deleted_dossiers_states' do subject { Stat.send(:deleted_dossiers_states) } @@ -78,19 +80,21 @@ describe Stat do describe '.last_four_months_hash' do it 'works count and cumulate counters by month for both dossier and deleted dossiers' do - 4.downto(1).map do |i| - create(:dossier, state: :en_construction, en_construction_at: i.months.ago) - create(:deleted_dossier, dossier_id: i + 100, state: :en_construction, deleted_at: i.month.ago) + travel_to Time.zone.local(2021, 11, 25) do + 4.downto(1).map do |i| + create(:dossier, state: :en_construction, en_construction_at: i.months.ago) + create(:deleted_dossier, dossier_id: i + 100, state: :en_construction, deleted_at: i.month.ago) + end + rs = Stat.send(:last_four_months_hash, [ + [Dossier.state_not_brouillon, :en_construction_at], + [DeletedDossier.where.not(state: :brouillon), :deleted_at] + ]) + expect(rs).to eq([ + ["août 2021", 2], + ["septembre 2021", 2], + ["octobre 2021", 2] + ]) end - rs = Stat.send(:last_four_months_hash, [ - [Dossier.state_not_brouillon, :en_construction_at], - [DeletedDossier.where.not(state: :brouillon), :deleted_at] - ]) - expect(rs).to eq([ - ["août 2021", 2], - ["septembre 2021", 2], - ["octobre 2021", 2] - ]) end end From 46e2e34b89f4daad616939a09780078aefaa5fa2 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Wed, 1 Dec 2021 17:34:00 +0100 Subject: [PATCH 3/9] fix(stats): remove super admin stats --- app/controllers/stats_controller.rb | 256 +--------------------- app/views/stats/index.html.haml | 17 -- spec/controllers/stats_controller_spec.rb | 107 --------- 3 files changed, 3 insertions(+), 377 deletions(-) diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index 32ad04935..73a253752 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -7,7 +7,6 @@ class StatsController < ApplicationController stat = Stat.first procedures = Procedure.publiees_ou_closes - dossiers = Dossier.state_not_brouillon @procedures_numbers = procedures_numbers(procedures) @@ -17,7 +16,9 @@ class StatsController < ApplicationController stat.dossiers_deposes_entre_60_et_30_jours ) - @contact_percentage = contact_percentage + @contact_percentage = Rails.cache.fetch("stats.contact_percentage", expires_in: 1.day) do + contact_percentage + end @dossiers_states_for_pie = { "Brouillon" => stat.dossiers_brouillon, @@ -31,25 +32,6 @@ class StatsController < ApplicationController @dossiers_cumulative = stat.dossiers_cumulative @dossiers_in_the_last_4_months = stat.dossiers_in_the_last_4_months - - if super_admin_signed_in? - @dossier_instruction_mean_time = Rails.cache.fetch("dossier_instruction_mean_time", expires_in: 1.day) do - dossier_instruction_mean_time(dossiers) - end - - @dossier_filling_mean_time = Rails.cache.fetch("dossier_filling_mean_time", expires_in: 1.day) do - dossier_filling_mean_time(dossiers) - end - - @avis_usage = avis_usage - @avis_average_answer_time = avis_average_answer_time - @avis_answer_percentages = avis_answer_percentages - - @motivation_usage_dossier = motivation_usage_dossier - @motivation_usage_procedure = motivation_usage_procedure - - @cloned_from_library_procedures_ratio = cloned_from_library_procedures_ratio - end end def download @@ -146,22 +128,6 @@ class StatsController < ApplicationController end end - def cloned_from_library_procedures_ratio - [3.weeks.ago, 2.weeks.ago, 1.week.ago].map do |date| - min_date = date.beginning_of_week - max_date = min_date.end_of_week - - all_procedures = Procedure.created_during(min_date..max_date) - cloned_from_library_procedures = all_procedures.cloned_from_library - - denominator = [1, all_procedures.count].max - - ratio = percentage(cloned_from_library_procedures.count, denominator) - - [l(max_date, format: '%d/%m/%Y'), ratio] - end - end - def max_date if super_admin_signed_in? Time.zone.now @@ -193,220 +159,4 @@ class StatsController < ApplicationController .map { |x, y| { x => (sum += y) } } .reduce({}, :merge) end - - def mean(collection) - (collection.sum.to_f / collection.size).round(2) - end - - def percentage(numerator, denominator) - ((numerator.to_f / denominator) * 100).round(2) - end - - def dossier_instruction_mean_time(dossiers) - # In the 12 last months, we compute for each month - # the average time it took to instruct a dossier - # We compute monthly averages by first making an average per procedure - # and then computing the average for all the procedures - - min_date = 11.months.ago - max_date = Time.zone.now.to_date - - processed_dossiers = Traitement.includes(:dossier) - .where(dossier_id: dossiers) - .where('dossiers.state' => Dossier::TERMINE) - .where(:processed_at => min_date..max_date) - .pluck('dossiers.groupe_instructeur_id', 'dossiers.en_construction_at', :processed_at) - - # Group dossiers by month - processed_dossiers_by_month = processed_dossiers - .group_by do |dossier| - dossier[2].beginning_of_month.to_s - end - - processed_dossiers_by_month.map do |month, value| - # Group the dossiers for this month by procedure - dossiers_grouped_by_groupe_instructeur = value.group_by { |dossier| dossier[0] } - - # Compute the mean time for this procedure - procedure_processing_times = dossiers_grouped_by_groupe_instructeur.map do |_procedure_id, procedure_dossiers| - procedure_dossiers_processing_time = procedure_dossiers.map do |dossier| - (dossier[2] - dossier[1]).to_f / (3600 * 24) - end - - mean(procedure_dossiers_processing_time) - end - - # Compute the average mean time for all the procedures of this month - month_average = mean(procedure_processing_times) - - [month, month_average] - end.to_h - end - - def dossier_filling_mean_time(dossiers) - # In the 12 last months, we compute for each month - # the average time it took to fill a dossier - # We compute monthly averages by first making an average per procedure - # and then computing the average for all the procedures - # For each procedure, we normalize the data: the time is calculated - # for a 24 champs form (the current form mean length) - - min_date = 11.months.ago - max_date = Time.zone.now.to_date - - processed_dossiers = Traitement.includes(:dossier) - .where(dossier: dossiers) - .where('dossiers.state' => Dossier::TERMINE) - .where(:processed_at => min_date..max_date) - .pluck( - 'dossiers.groupe_instructeur_id', - Arel.sql('EXTRACT(EPOCH FROM (dossiers.en_construction_at - dossiers.created_at)) / 60 AS processing_time'), - :processed_at - ) - - # Group dossiers by month - processed_dossiers_by_month = processed_dossiers - .group_by do |(*_, processed_at)| - processed_at.beginning_of_month.to_s - end - - groupe_instructeur_ids = processed_dossiers.map { |gid, _, _| gid }.uniq - groupe_instructeurs = GroupeInstructeur.where(id: groupe_instructeur_ids).pluck(:id, :procedure_id) - - procedure_id_type_de_champs_count = TypeDeChamp - .where(private: false) - .joins(:revision) - .group('procedure_revisions.procedure_id') - .count - - groupe_instructeur_id_type_de_champs_count = groupe_instructeurs.reduce({}) do |acc, (gi_id, procedure_id)| - acc[gi_id] = procedure_id_type_de_champs_count[procedure_id] - acc - end - - processed_dossiers_by_month.map do |month, dossier_plucks| - # Group the dossiers for this month by procedure - dossiers_grouped_by_groupe_instructeur = dossier_plucks.group_by { |(groupe_instructeur_id, *_)| groupe_instructeur_id } - - # Compute the mean time for this procedure - procedure_processing_times = dossiers_grouped_by_groupe_instructeur.filter_map do |groupe_instructeur_id, procedure_dossiers| - procedure_fields_count = groupe_instructeur_id_type_de_champs_count[groupe_instructeur_id] - - if (procedure_fields_count == 0 || procedure_fields_count.nil?) - next - end - - procedure_dossiers_processing_time = procedure_dossiers.map { |_, processing_time, _| processing_time } - procedure_mean = mean(procedure_dossiers_processing_time) - - # We normalize the data for 24 fields - procedure_mean * (MEAN_NUMBER_OF_CHAMPS_IN_A_FORM / procedure_fields_count) - end - - # Compute the average mean time for all the procedures of this month - month_average = mean(procedure_processing_times) - - [month, month_average] - end.to_h - end - - def avis_usage - [3.weeks.ago, 2.weeks.ago, 1.week.ago].map do |min_date| - max_date = min_date + 1.week - - weekly_dossiers = Dossier.includes(:avis).where(created_at: min_date..max_date).to_a - - weekly_dossiers_count = weekly_dossiers.count - - if weekly_dossiers_count == 0 - result = 0 - else - weekly_dossier_with_avis_count = weekly_dossiers.count { |dossier| dossier.avis.present? } - result = percentage(weekly_dossier_with_avis_count, weekly_dossiers_count) - end - - [min_date.to_i, result] - end - end - - def avis_average_answer_time - [3.weeks.ago, 2.weeks.ago, 1.week.ago].map do |min_date| - max_date = min_date + 1.week - - average = Avis.with_answer - .where(created_at: min_date..max_date) - .average("EXTRACT(EPOCH FROM avis.updated_at - avis.created_at) / 86400") - - result = average ? average.to_f.round(2) : 0 - - [min_date.to_i, result] - end - end - - def avis_answer_percentages - [3.weeks.ago, 2.weeks.ago, 1.week.ago].map do |min_date| - max_date = min_date + 1.week - - weekly_avis = Avis.where(created_at: min_date..max_date) - - weekly_avis_count = weekly_avis.count - - if weekly_avis_count == 0 - [min_date.to_i, 0] - else - answered_weekly_avis_count = weekly_avis.with_answer.count - result = percentage(answered_weekly_avis_count, weekly_avis_count) - - [min_date.to_i, result] - end - end - end - - def motivation_usage_dossier - [3.weeks.ago, 2.weeks.ago, 1.week.ago].map do |date| - min_date = date.beginning_of_week - max_date = date.end_of_week - - weekly_termine_dossiers = Dossier.where(processed_at: min_date..max_date) - weekly_termine_dossiers_count = weekly_termine_dossiers.count - weekly_termine_dossiers_with_motivation_count = weekly_termine_dossiers.where.not(motivation: nil).count - - if weekly_termine_dossiers_count == 0 - result = 0 - else - result = percentage(weekly_termine_dossiers_with_motivation_count, weekly_termine_dossiers_count) - end - - [l(max_date, format: '%d/%m/%Y'), result] - end - end - - def motivation_usage_procedure - [3.weeks.ago, 2.weeks.ago, 1.week.ago].map do |date| - min_date = date.beginning_of_week - max_date = date.end_of_week - - procedures_with_dossier_processed_this_week = Procedure - .joins(:dossiers) - .where(dossiers: { processed_at: min_date..max_date }) - - procedures_with_dossier_processed_this_week_count = procedures_with_dossier_processed_this_week - .uniq - .count - - procedures_with_dossier_processed_this_week_and_with_motivation_count = procedures_with_dossier_processed_this_week - .where - .not(dossiers: { motivation: nil }) - .uniq - .count - - if procedures_with_dossier_processed_this_week_count == 0 - result = 0 - else - result = percentage(procedures_with_dossier_processed_this_week_and_with_motivation_count, procedures_with_dossier_processed_this_week_count) - end - - [l(max_date, format: '%d/%m/%Y'), result] - end - end end diff --git a/app/views/stats/index.html.haml b/app/views/stats/index.html.haml index 9704f56de..2c27ba271 100644 --- a/app/views/stats/index.html.haml +++ b/app/views/stats/index.html.haml @@ -75,23 +75,6 @@ .chart.cumulative-dossiers-chart.hidden = area_chart @dossiers_cumulative - - if super_admin_signed_in? - .stat-card.stat-card-half.pull-left - %span.stat-card-title Temps de traitement moyen d’un dossier - - .chart-container - .chart - = line_chart @dossier_instruction_mean_time, - :ytitle => "Jours" - - .stat-card.stat-card-half.pull-left - %span.stat-card-title Temps de remplissage moyen d’un dossier - - .chart-container - .chart - = line_chart @dossier_filling_mean_time, - :ytitle => "Minutes" - .clearfix - if super_admin_signed_in? diff --git a/spec/controllers/stats_controller_spec.rb b/spec/controllers/stats_controller_spec.rb index ab80cf07e..bb6c11914 100644 --- a/spec/controllers/stats_controller_spec.rb +++ b/spec/controllers/stats_controller_spec.rb @@ -90,111 +90,4 @@ describe StatsController, type: :controller do end end end - - describe "#dossier_instruction_mean_time" do - # Month-2: mean 3 days - # procedure_1: mean 2 days - # dossier_p1_a: 3 days - # dossier_p1_b: 1 days - # procedure_2: mean 4 days - # dossier_p2_a: 4 days - # - # Month-1: mean 5 days - # procedure_1: mean 5 days - # dossier_p1_c: 5 days - - before do - procedure_1 = create(:procedure) - procedure_2 = create(:procedure) - dossier_p1_a = create(:dossier, :accepte, - procedure: procedure_1, - en_construction_at: 2.months.ago.beginning_of_month, - processed_at: 2.months.ago.beginning_of_month + 3.days) - dossier_p1_b = create(:dossier, :accepte, - procedure: procedure_1, - en_construction_at: 2.months.ago.beginning_of_month, - processed_at: 2.months.ago.beginning_of_month + 1.day) - dossier_p1_c = create(:dossier, :accepte, - procedure: procedure_1, - en_construction_at: 1.month.ago.beginning_of_month, - processed_at: 1.month.ago.beginning_of_month + 5.days) - dossier_p2_a = create(:dossier, :accepte, - procedure: procedure_2, - en_construction_at: 2.months.ago.beginning_of_month, - processed_at: 2.months.ago.beginning_of_month + 4.days) - - @expected_hash = { - (2.months.ago.beginning_of_month).to_s => 3.0, - (1.month.ago.beginning_of_month).to_s => 5.0 - } - end - - let (:association) { Dossier.state_not_brouillon } - - subject { StatsController.new.send(:dossier_instruction_mean_time, association) } - - it { expect(subject).to eq(@expected_hash) } - end - - describe "#dossier_filling_mean_time" do - # Month-2: mean 30 minutes - # procedure_1: mean 20 minutes - # dossier_p1_a: 30 minutes - # dossier_p1_b: 10 minutes - # procedure_2: mean 40 minutes - # dossier_p2_a: 80 minutes, for twice the fields - # - # Month-1: mean 50 minutes - # procedure_1: mean 50 minutes - # dossier_p1_c: 50 minutes - - before do - procedure_1 = create(:procedure, :with_type_de_champ, types_de_champ_count: 24) - procedure_2 = create(:procedure, :with_type_de_champ, types_de_champ_count: 48) - dossier_p1_a = create(:dossier, :accepte, - procedure: procedure_1, - created_at: 2.months.ago.beginning_of_month, - en_construction_at: 2.months.ago.beginning_of_month + 30.minutes, - processed_at: 2.months.ago.beginning_of_month + 1.day) - dossier_p1_b = create(:dossier, :accepte, - procedure: procedure_1, - created_at: 2.months.ago.beginning_of_month, - en_construction_at: 2.months.ago.beginning_of_month + 10.minutes, - processed_at: 2.months.ago.beginning_of_month + 1.day) - dossier_p1_c = create(:dossier, :accepte, - procedure: procedure_1, - created_at: 1.month.ago.beginning_of_month, - en_construction_at: 1.month.ago.beginning_of_month + 50.minutes, - processed_at: 1.month.ago.beginning_of_month + 1.day) - dossier_p2_a = create(:dossier, :accepte, - procedure: procedure_2, - created_at: 2.months.ago.beginning_of_month, - en_construction_at: 2.months.ago.beginning_of_month + 80.minutes, - processed_at: 2.months.ago.beginning_of_month + 1.day) - - @expected_hash = { - (2.months.ago.beginning_of_month).to_s => 30.0, - (1.month.ago.beginning_of_month).to_s => 50.0 - } - end - - let (:association) { Dossier.state_not_brouillon } - - subject { StatsController.new.send(:dossier_filling_mean_time, association) } - - it { expect(subject).to eq(@expected_hash) } - end - - describe '#avis_usage' do - let!(:dossier) { create(:dossier) } - let!(:avis_with_dossier) { create(:avis) } - let!(:dossier2) { create(:dossier) } - - before { Timecop.freeze(Time.zone.now) } - after { Timecop.return } - - subject { StatsController.new.send(:avis_usage) } - - it { expect(subject).to match([[3.weeks.ago.to_i, 0], [2.weeks.ago.to_i, 0], [1.week.ago.to_i, 33.33]]) } - end end From ac07f05771f8f77c0cd99d165aa972d8fb76f1a0 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 25 Nov 2021 15:37:40 +0100 Subject: [PATCH 4/9] app: use Instructeur.bypass_email_login_token --- app/controllers/application_controller.rb | 2 +- spec/controllers/application_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 76c3583bc..03c5e3dbb 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -224,7 +224,7 @@ class ApplicationController < ActionController::Base def redirect_if_untrusted if instructeur_signed_in? && sensitive_path && - !feature_enabled?(:instructeur_bypass_email_login_token) && + !current_instructeur.bypass_email_login_token && !IPService.ip_trusted?(request.headers['X-Forwarded-For']) && !trusted_device? diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 488c71a9d..e7a43ce8b 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -165,7 +165,7 @@ describe ApplicationController, type: :controller do let(:sensitive_path) { true } before do - Flipper.disable(:instructeur_bypass_email_login_token) + current_instructeur.update!(bypass_email_login_token: false) end context 'when the instructeur is signed_in' do From 8b3d31980a2458d93fc4b145b8f4b440bc47b515 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 25 Nov 2021 15:38:01 +0100 Subject: [PATCH 5/9] specs: use Instructeur.bypass_email_login_token in the tests --- spec/factories/instructeur.rb | 2 ++ spec/rails_helper.rb | 4 ---- spec/support/system_helpers.rb | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/spec/factories/instructeur.rb b/spec/factories/instructeur.rb index 11f28a5d5..b913dd7c9 100644 --- a/spec/factories/instructeur.rb +++ b/spec/factories/instructeur.rb @@ -2,6 +2,8 @@ FactoryBot.define do sequence(:instructeur_email) { |n| "inst#{n}@inst.com" } factory :instructeur do + bypass_email_login_token { true } + user { association :user, email: email, password: password } transient do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 705f4d462..b58e5553d 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -89,10 +89,6 @@ RSpec.configure do |config| Geocoder.configure(lookup: :test) end - config.before(:each) do - Flipper.enable(:instructeur_bypass_email_login_token) - end - # By default, forgery protection is disabled in the test environment. # (See `config.action_controller.allow_forgery_protection` in `config/test.rb`) # diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb index 0db4c350c..7c06cec6b 100644 --- a/spec/support/system_helpers.rb +++ b/spec/support/system_helpers.rb @@ -17,7 +17,7 @@ module SystemHelpers fill_in :user_password, with: password if sign_in_by_link - Flipper.disable(:instructeur_bypass_email_login_token) + User.find_by(email: email)&.instructeur&.update!(bypass_email_login_token: false) end perform_enqueued_jobs do From d524beee4ea71c683a56fe33d5d08c2dacd49e83 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 25 Nov 2021 14:50:23 +0000 Subject: [PATCH 6/9] app: remove `:instructeur_bypass_email_login_token` from Flipper --- .../manager/instructeurs_controller.rb | 18 ------------------ config/application.rb | 4 ++-- config/initializers/flipper.rb | 1 - 3 files changed, 2 insertions(+), 21 deletions(-) diff --git a/app/controllers/manager/instructeurs_controller.rb b/app/controllers/manager/instructeurs_controller.rb index b69b89d35..2bfe85d9c 100644 --- a/app/controllers/manager/instructeurs_controller.rb +++ b/app/controllers/manager/instructeurs_controller.rb @@ -1,23 +1,5 @@ module Manager class InstructeursController < Manager::ApplicationController - # Temporary code: synchronize Flipper's instructeur_bypass_email_login_token - # when Instructeur.bypass_email_login_token is modified. - # - # This will be removed when the migration of this feature flag out of Flipper will be complete. - def update - super - - instructeur = requested_resource - saved_successfully = !requested_resource.changed? - if saved_successfully - if instructeur.bypass_email_login_token - Flipper.enable_actor(:instructeur_bypass_email_login_token, instructeur.user) - else - Flipper.disable_actor(:instructeur_bypass_email_login_token, instructeur.user) - end - end - end - def reinvite instructeur = Instructeur.find(params[:id]) instructeur.user.invite! diff --git a/config/application.rb b/config/application.rb index e680d7ac0..2d9b15e9f 100644 --- a/config/application.rb +++ b/config/application.rb @@ -63,8 +63,8 @@ module TPS end config.middleware.use Rack::Attack - config.middleware.use Flipper::Middleware::Memoizer, - preload: [:instructeur_bypass_email_login_token] + # Ensure we make maximum one call per feature per request. + config.middleware.use Flipper::Middleware::Memoizer config.ds_env = ENV.fetch('DS_ENV', Rails.env) diff --git a/config/initializers/flipper.rb b/config/initializers/flipper.rb index 1dd25d5cd..f335c7ce3 100644 --- a/config/initializers/flipper.rb +++ b/config/initializers/flipper.rb @@ -30,7 +30,6 @@ features = [ :dossier_pdf_vide, :expert_not_allowed_to_invite, :hide_instructeur_email, - :instructeur_bypass_email_login_token, :procedure_revisions, :procedure_routage_api, :procedure_process_expired_dossiers_termine From 115ca5e24a79f72bdff74d66863bbe784b6e4770 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 30 Nov 2021 09:17:19 +0100 Subject: [PATCH 7/9] js: don't create invalid menus by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a new "Menu" type de champ is added, it comes pre-filled with a menu title – and nothing else. Which is confusing, and invalid. Instead pre-fill the type de champ with actual values (no titles). --- .../components/TypesDeChampEditor/typeDeChampsReducer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/javascript/components/TypesDeChampEditor/typeDeChampsReducer.js b/app/javascript/components/TypesDeChampEditor/typeDeChampsReducer.js index 4a5cdaede..81125b54a 100644 --- a/app/javascript/components/TypesDeChampEditor/typeDeChampsReducer.js +++ b/app/javascript/components/TypesDeChampEditor/typeDeChampsReducer.js @@ -110,7 +110,7 @@ function updateTypeDeChamp( break; case 'drop_down_list': case 'multiple_drop_down_list': - typeDeChamp.drop_down_list_value = '--Premier élément du menu--\n'; + typeDeChamp.drop_down_list_value = 'Premier choix\nDeuxième choix'; } } From e219ec33d85e7779ff3f9e66a1d79c666607a6e0 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 30 Nov 2021 12:23:46 +0100 Subject: [PATCH 8/9] validators: rename no_empty_repetitions to no_empty_repetition --- app/models/procedure.rb | 2 +- ...epetitions_validator.rb => no_empty_repetition_validator.rb} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename app/validators/revisions/{no_empty_repetitions_validator.rb => no_empty_repetition_validator.rb} (89%) diff --git a/app/models/procedure.rb b/app/models/procedure.rb index cd4ac5ad1..aacf4783e 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -234,7 +234,7 @@ class Procedure < ApplicationRecord validates :description, presence: true, allow_blank: false, allow_nil: false validates :administrateurs, presence: true validates :lien_site_web, presence: true, if: :publiee? - validates :draft_revision, 'revisions/no_empty_repetitions': true, if: :validate_for_publication? + validates :draft_revision, 'revisions/no_empty_repetition': true, if: :validate_for_publication? validate :check_juridique validates :path, presence: true, format: { with: /\A[a-z0-9_\-]{3,200}\z/ }, uniqueness: { scope: [:path, :closed_at, :hidden_at, :unpublished_at], case_sensitive: false } validates :duree_conservation_dossiers_dans_ds, allow_nil: false, numericality: { only_integer: true, greater_than_or_equal_to: 1, less_than_or_equal_to: MAX_DUREE_CONSERVATION } diff --git a/app/validators/revisions/no_empty_repetitions_validator.rb b/app/validators/revisions/no_empty_repetition_validator.rb similarity index 89% rename from app/validators/revisions/no_empty_repetitions_validator.rb rename to app/validators/revisions/no_empty_repetition_validator.rb index b7a44043e..ba8bf85c7 100644 --- a/app/validators/revisions/no_empty_repetitions_validator.rb +++ b/app/validators/revisions/no_empty_repetition_validator.rb @@ -1,4 +1,4 @@ -class Revisions::NoEmptyRepetitionsValidator < ActiveModel::EachValidator +class Revisions::NoEmptyRepetitionValidator < ActiveModel::EachValidator def validate_each(procedure, attribute, revision) return if revision.nil? From b7d17b09891f95bf34d61d81b680ddc4b8cae490 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 30 Nov 2021 12:26:19 +0100 Subject: [PATCH 9/9] models: validate that no drop-downs are empty on publishing Disallow publishing a procedure containing drop-downs with no selectable values. --- app/models/procedure.rb | 5 +++- .../revisions/no_empty_drop_down_validator.rb | 22 +++++++++++++++++ config/locales/fr.yml | 1 + spec/factories/type_de_champ.rb | 3 +++ spec/models/procedure_spec.rb | 24 ++++++++++++++++--- 5 files changed, 51 insertions(+), 4 deletions(-) create mode 100644 app/validators/revisions/no_empty_drop_down_validator.rb diff --git a/app/models/procedure.rb b/app/models/procedure.rb index aacf4783e..e85548ce1 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -234,7 +234,10 @@ class Procedure < ApplicationRecord validates :description, presence: true, allow_blank: false, allow_nil: false validates :administrateurs, presence: true validates :lien_site_web, presence: true, if: :publiee? - validates :draft_revision, 'revisions/no_empty_repetition': true, if: :validate_for_publication? + validates :draft_revision, + 'revisions/no_empty_repetition': true, + 'revisions/no_empty_drop_down': true, + if: :validate_for_publication? validate :check_juridique validates :path, presence: true, format: { with: /\A[a-z0-9_\-]{3,200}\z/ }, uniqueness: { scope: [:path, :closed_at, :hidden_at, :unpublished_at], case_sensitive: false } validates :duree_conservation_dossiers_dans_ds, allow_nil: false, numericality: { only_integer: true, greater_than_or_equal_to: 1, less_than_or_equal_to: MAX_DUREE_CONSERVATION } diff --git a/app/validators/revisions/no_empty_drop_down_validator.rb b/app/validators/revisions/no_empty_drop_down_validator.rb new file mode 100644 index 000000000..86e257e34 --- /dev/null +++ b/app/validators/revisions/no_empty_drop_down_validator.rb @@ -0,0 +1,22 @@ +class Revisions::NoEmptyDropDownValidator < ActiveModel::EachValidator + def validate_each(procedure, attribute, revision) + return if revision.nil? + + tdcs = revision.types_de_champ + revision.types_de_champ_private + drop_downs = tdcs.filter(&:drop_down_list?) + drop_downs.each do |drop_down| + validate_drop_down_not_empty(procedure, attribute, drop_down) + end + end + + private + + def validate_drop_down_not_empty(procedure, attribute, drop_down) + if drop_down.drop_down_list_enabled_non_empty_options.empty? + procedure.errors.add( + attribute, + procedure.errors.generate_message(attribute, :empty_drop_down, { value: drop_down.libelle }) + ) + end + end +end diff --git a/config/locales/fr.yml b/config/locales/fr.yml index a777d405a..b44788763 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -311,6 +311,7 @@ fr: forbidden_html: "Seul-e-s les usagers peuvent se connecter via France Connect. En tant qu’instructeur ou administrateur, nous vous invitons à réininitialiser votre mot de passe." procedure_archived: "Cette démarche en ligne a été close, il n’est plus possible de déposer de dossier." empty_repetition: 'Le bloc répétable « %{value} » doit comporter au moins un champ' + empty_drop_down: 'La liste de choix « %{value} » doit comporter au moins un choix sélectionnable' # procedure_not_draft: "Cette démarche n’est maintenant plus en brouillon." cadastres_empty: one: "Aucune parcelle cadastrale sur la zone sélectionnée" diff --git a/spec/factories/type_de_champ.rb b/spec/factories/type_de_champ.rb index 9f5223e86..ea9ea2a8c 100644 --- a/spec/factories/type_de_champ.rb +++ b/spec/factories/type_de_champ.rb @@ -89,6 +89,9 @@ FactoryBot.define do trait :long do drop_down_list_value { "alpha\r\nbravo\r\n--separateur--\r\ncharly\r\ndelta\r\necho\r\nfox-trot\r\ngolf" } end + trait :without_selectable_values do + drop_down_list_value { "\r\n--separateur--\r\n--separateur 2--\r\n \r\n" } + end end factory :type_de_champ_multiple_drop_down_list do type_champ { TypeDeChamp.type_champs.fetch(:multiple_drop_down_list) } diff --git a/spec/models/procedure_spec.rb b/spec/models/procedure_spec.rb index 2ff4a2ff9..146c98926 100644 --- a/spec/models/procedure_spec.rb +++ b/spec/models/procedure_spec.rb @@ -282,13 +282,17 @@ describe Procedure do describe 'draft_revision' do let(:repetition) { build(:type_de_champ_repetition, libelle: 'Enfants') } let(:text_field) { build(:type_de_champ_text) } - let(:procedure) { create(:procedure, types_de_champ: [repetition]) } let(:invalid_repetition_error_message) { 'Le bloc répétable « Enfants » doit comporter au moins un champ' } + let(:drop_down) { build(:type_de_champ_drop_down_list, :without_selectable_values, libelle: 'Civilité') } + let(:invalid_drop_down_error_message) { 'La liste de choix « Civilité » doit comporter au moins un choix sélectionnable' } + + let(:procedure) { create(:procedure, types_de_champ: [repetition, drop_down]) } + context 'on a draft procedure' do - it 'doesn’t validate repetitions' do + it 'doesn’t validate the draft revision' do procedure.validate - expect(procedure.errors[:draft_revision]).not_to include(invalid_repetition_error_message) + expect(procedure.errors[:draft_revision]).not_to be_present end end @@ -306,6 +310,15 @@ describe Procedure do procedure.validate expect(procedure.errors.full_messages_for(:draft_revision)).not_to include(invalid_repetition_error_message) end + + it 'validates that no drop-down type de champ is empty' do + procedure.validate + expect(procedure.errors.full_messages_for(:draft_revision)).to include(invalid_drop_down_error_message) + + drop_down.update!(drop_down_list_value: "--title--\r\nsome value") + procedure.reload.validate + expect(procedure.errors.full_messages_for(:draft_revision)).not_to include(invalid_drop_down_error_message) + end end context 'when validating for publication' do @@ -313,6 +326,11 @@ describe Procedure do procedure.validate(:publication) expect(procedure.errors.full_messages_for(:draft_revision)).to include(invalid_repetition_error_message) end + + it 'validates that no drop-down type de champ is empty' do + procedure.validate(:publication) + expect(procedure.errors.full_messages_for(:draft_revision)).to include(invalid_drop_down_error_message) + end end end end