From 970e43efb8ffbffe10d27cfa0110c33efe939c37 Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 25 Nov 2021 09:11:05 +0100 Subject: [PATCH] feat(stats#index): update Stat model to also query DossierDeleted in stats computation tech(question): discard_and_keep_track! ; are we really keeping track with default_scope { kept } ? feat(stats): add DeletedDossier in Stat computations Revert "tech(question): discard_and_keep_track! ; are we really keeping track with default_scope { kept } ?" This reverts commit d1155b7eeaaf1a9f80189e59667e109541fcb089. feat(stats): support deleted_dossiers for last_four_months_hash and cumulative_hash. extract sanitize query & merge hashes in methdos clean(rubocop): lint with rubocop Update db/migrate/20211126080118_add_index_to_deleted_at_to_deleted_dossiers.rb Co-authored-by: LeSim fix(rubocop): avoid uneeded allocation fix(migration): add concurrent index with expected synthax fix(brakeman): add ignore message since group date_trunc evaluation is used by only ourself --- app/models/stat.rb | 102 ++++++++++++------ config/brakeman.ignore | 44 +++++++- ...index_to_deleted_at_to_deleted_dossiers.rb | 11 ++ db/schema.rb | 3 +- spec/models/stat_spec.rb | 102 ++++++++++++++++++ 5 files changed, 225 insertions(+), 37 deletions(-) create mode 100644 db/migrate/20211126080118_add_index_to_deleted_at_to_deleted_dossiers.rb create mode 100644 spec/models/stat_spec.rb diff --git a/app/models/stat.rb b/app/models/stat.rb index 50d68a105..dfc893caf 100644 --- a/app/models/stat.rb +++ b/app/models/stat.rb @@ -19,7 +19,7 @@ class Stat < ApplicationRecord class << self def update_stats - states = dossiers_states + states = sum_hashes(dossiers_states, deleted_dossiers_states) stat = Stat.first || Stat.new stat.update( @@ -30,8 +30,14 @@ class Stat < ApplicationRecord dossiers_deposes_entre_60_et_30_jours: states['dossiers_deposes_entre_60_et_30_jours'], dossiers_not_brouillon: states['not_brouillon'], dossiers_termines: states['termines'], - dossiers_cumulative: cumulative_hash(Dossier.state_not_brouillon, :en_construction_at), - dossiers_in_the_last_4_months: last_four_months_hash(Dossier.state_not_brouillon, :en_construction_at), + dossiers_cumulative: cumulative_hash([ + [Dossier.state_not_brouillon, :en_construction_at], + [DeletedDossier.where.not(state: :brouillon), :deleted_at] + ]), + dossiers_in_the_last_4_months: last_four_months_hash([ + [Dossier.state_not_brouillon, :en_construction_at], + [DeletedDossier.where.not(state: :brouillon), :deleted_at] + ]), administrations_partenaires: AdministrateursProcedure.joins(:procedure).merge(Procedure.publiees_ou_closes).select('distinct administrateur_id').count ) end @@ -39,55 +45,83 @@ class Stat < ApplicationRecord private def dossiers_states - query = <<-EOF - SELECT - COUNT(*) FILTER ( WHERE state != 'brouillon' ) AS "not_brouillon", - COUNT(*) FILTER ( WHERE state != 'brouillon' and en_construction_at BETWEEN :one_month_ago AND :now ) AS "dossiers_depose_avant_30_jours", - COUNT(*) FILTER ( WHERE state != 'brouillon' and en_construction_at BETWEEN :two_months_ago AND :one_month_ago ) AS "dossiers_deposes_entre_60_et_30_jours", - COUNT(*) FILTER ( WHERE state = 'brouillon' ) AS "brouillon", - COUNT(*) FILTER ( WHERE state = 'en_construction' ) AS "en_construction", - COUNT(*) FILTER ( WHERE state = 'en_instruction' ) AS "en_instruction", - COUNT(*) FILTER ( WHERE state in ('accepte', 'refuse', 'sans_suite') ) AS "termines" - FROM dossiers - WHERE hidden_at IS NULL + sanitize_and_exec(Dossier, <<-EOF + SELECT + COUNT(*) FILTER ( WHERE state != 'brouillon' ) AS "not_brouillon", + COUNT(*) FILTER ( WHERE state != 'brouillon' and en_construction_at BETWEEN :one_month_ago AND :now ) AS "dossiers_depose_avant_30_jours", + COUNT(*) FILTER ( WHERE state != 'brouillon' and en_construction_at BETWEEN :two_months_ago AND :one_month_ago ) AS "dossiers_deposes_entre_60_et_30_jours", + COUNT(*) FILTER ( WHERE state = 'brouillon' ) AS "brouillon", + COUNT(*) FILTER ( WHERE state = 'en_construction' ) AS "en_construction", + COUNT(*) FILTER ( WHERE state = 'en_instruction' ) AS "en_instruction", + COUNT(*) FILTER ( WHERE state in ('accepte', 'refuse', 'sans_suite') ) AS "termines" + FROM dossiers + WHERE hidden_at IS NULL EOF - - sanitized_query = ActiveRecord::Base.sanitize_sql([ - query, - now: Time.zone.now, - one_month_ago: 1.month.ago, - two_months_ago: 2.months.ago - ]) - - Dossier.connection.select_all(sanitized_query).first + ) end - def last_four_months_hash(association, date_attribute) - min_date = 3.months.ago.beginning_of_month.to_date + def deleted_dossiers_states + sanitize_and_exec(DeletedDossier, <<-EOF + SELECT + COUNT(*) FILTER ( WHERE state != 'brouillon' ) AS "not_brouillon", + COUNT(*) FILTER ( WHERE state != 'brouillon' and deleted_at BETWEEN :one_month_ago AND :now ) AS "dossiers_depose_avant_30_jours", + COUNT(*) FILTER ( WHERE state != 'brouillon' and deleted_at BETWEEN :two_months_ago AND :one_month_ago ) AS "dossiers_deposes_entre_60_et_30_jours", + COUNT(*) FILTER ( WHERE state = 'brouillon' ) AS "brouillon", + COUNT(*) FILTER ( WHERE state = 'en_construction' ) AS "en_construction", + COUNT(*) FILTER ( WHERE state = 'en_instruction' ) AS "en_instruction", + COUNT(*) FILTER ( WHERE state in ('accepte', 'refuse', 'sans_suite') ) AS "termines" + FROM deleted_dossiers + EOF + ) + end - association - .where(date_attribute => min_date..max_date) - .group("DATE_TRUNC('month', #{date_attribute})") - .count + def last_four_months_hash(associations_with_date_attribute) + min_date = 3.months.ago.beginning_of_month.to_date + timeseries = associations_with_date_attribute.map do |association, date_attribute| + association + .where(date_attribute => min_date..max_date) + .group("DATE_TRUNC('month', #{date_attribute})") + .count + end + + sum_hashes(*timeseries) .to_a .sort_by { |a| a[0] } .map { |e| [I18n.l(e.first, format: "%B %Y"), e.last] } end - def cumulative_hash(association, date_attribute) + def cumulative_hash(associations_with_date_attribute) + timeseries = associations_with_date_attribute.map do |association, date_attribute| + association + .where("#{date_attribute} < ?", max_date) + .group("DATE_TRUNC('month', #{date_attribute})") + .count + end + sum = 0 - association - .where("#{date_attribute} < ?", max_date) - .group("DATE_TRUNC('month', #{date_attribute})") - .count + sum_hashes(*timeseries) .to_a .sort_by { |a| a[0] } .map { |x, y| { x => (sum += y) } } .reduce({}, :merge) end + def sum_hashes(*hashes) + {}.merge(*hashes) { |_k, hash_one_value, hash_two_value| hash_one_value + hash_two_value } + end + def max_date Time.zone.now.beginning_of_month - 1.second end + + def sanitize_and_exec(model, query) + sanitized_query = ActiveRecord::Base.sanitize_sql([ + query, + now: Time.zone.now, + one_month_ago: 1.month.ago, + two_months_ago: 2.months.ago + ]) + model.connection.select_all(sanitized_query).first + end end end diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 46908df59..f718aba91 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -46,7 +46,7 @@ "type": "controller", "class": "Users::DossiersController", "method": "merci", - "line": 193, + "line": 195, "file": "app/controllers/users/dossiers_controller.rb", "rendered": { "name": "users/dossiers/merci", @@ -62,6 +62,26 @@ "confidence": "Weak", "note": "" }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "6c98e520dd368104bb0c81334875010711cd523afc28057ef86a10930f95c4b7", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/stat.rb", + "line": 83, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "association.where(date_attribute => ((3.months.ago.beginning_of_month.to_date..max_date))).group(\"DATE_TRUNC('month', #{date_attribute})\")", + "render_path": null, + "location": { + "type": "method", + "class": "Stat", + "method": "last_four_months_hash" + }, + "user_input": "date_attribute", + "confidence": "Weak", + "note": "no user input, fixed value" + }, { "warning_type": "SQL Injection", "warning_code": 0, @@ -101,8 +121,28 @@ "user_input": "Export.find_or_create_export(params[:export_format], (params[:time_span_type] or \"everything\"), current_instructeur.groupe_instructeurs.where(:procedure => procedure)).file.service_url", "confidence": "High", "note": "" + }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "dc6d873aff3dc5e51e3349b17e1f35039b23d0bddbf04224b0f1bca3e4608c1e", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/stat.rb", + "line": 97, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "association.where(\"#{date_attribute} < ?\", max_date).group(\"DATE_TRUNC('month', #{date_attribute})\")", + "render_path": null, + "location": { + "type": "method", + "class": "Stat", + "method": "cumulative_hash" + }, + "user_input": "date_attribute", + "confidence": "Weak", + "note": "no user input, fixed value" } ], - "updated": "2021-11-23 14:09:21 +0100", + "updated": "2021-11-26 13:22:41 +0100", "brakeman_version": "5.1.1" } diff --git a/db/migrate/20211126080118_add_index_to_deleted_at_to_deleted_dossiers.rb b/db/migrate/20211126080118_add_index_to_deleted_at_to_deleted_dossiers.rb new file mode 100644 index 000000000..3c59c70b2 --- /dev/null +++ b/db/migrate/20211126080118_add_index_to_deleted_at_to_deleted_dossiers.rb @@ -0,0 +1,11 @@ +class AddIndexToDeletedAtToDeletedDossiers < ActiveRecord::Migration[6.1] + include Database::MigrationHelpers + disable_ddl_transaction! + def up + add_concurrent_index :deleted_dossiers, :deleted_at + end + + def down + remove_index :deleted_dossiers, [:deleted_at] + end +end diff --git a/db/schema.rb b/db/schema.rb index 748e91ff5..d9eaf5fab 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_11_24_111429) do +ActiveRecord::Schema.define(version: 2021_11_26_080118) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -253,6 +253,7 @@ ActiveRecord::Schema.define(version: 2021_11_24_111429) do t.bigint "user_id" t.bigint "groupe_instructeur_id" t.bigint "revision_id" + t.index ["deleted_at"], name: "index_deleted_dossiers_on_deleted_at" t.index ["dossier_id"], name: "index_deleted_dossiers_on_dossier_id", unique: true t.index ["procedure_id"], name: "index_deleted_dossiers_on_procedure_id" end diff --git a/spec/models/stat_spec.rb b/spec/models/stat_spec.rb new file mode 100644 index 000000000..b03ae88c7 --- /dev/null +++ b/spec/models/stat_spec.rb @@ -0,0 +1,102 @@ +describe Stat do + describe '.deleted_dossiers_states' do + subject { Stat.send(:deleted_dossiers_states) } + it 'find counts for columns' do + create(:deleted_dossier, dossier_id: create(:dossier).id, state: :termine) + create(:deleted_dossier, dossier_id: create(:dossier).id, state: :en_construction, deleted_at: 1.month.ago) + create(:deleted_dossier, dossier_id: create(:dossier).id, state: :en_construction, deleted_at: 2.months.ago) + create(:deleted_dossier, dossier_id: create(:dossier).id, state: :brouillon, deleted_at: 3.months.ago) + create(:deleted_dossier, dossier_id: create(:dossier).id, state: :en_construction, deleted_at: 3.months.ago) + create(:deleted_dossier, dossier_id: create(:dossier).id, state: :en_instruction, deleted_at: 3.months.ago) + create(:deleted_dossier, dossier_id: create(:dossier).id, state: :accepte, deleted_at: 3.months.ago) + create(:deleted_dossier, dossier_id: create(:dossier).id, state: :refuse, deleted_at: 3.months.ago) + create(:deleted_dossier, dossier_id: create(:dossier).id, state: :sans_suite, deleted_at: 3.months.ago) + + expect(subject["not_brouillon"]).to eq(8) + expect(subject["dossiers_depose_avant_30_jours"]).to eq(1) + expect(subject["dossiers_deposes_entre_60_et_30_jours"]).to eq(1) + expect(subject["brouillon"]).to eq(1) + expect(subject["en_construction"]).to eq(3) + expect(subject["en_instruction"]).to eq(1) + expect(subject["termines"]).to eq(3) + end + end + + describe '.update_stats' do + it 'merges dossiers_states and deleted_dossiers_states' do + stats = { + "not_brouillon" => 1, + "dossiers_depose_avant_30_jours" => 2, + "dossiers_deposes_entre_60_et_30_jours" => 3, + "brouillon" => 4, + "en_construction" => 5, + "en_instruction" => 6, + "termines" => 7 + } + allow(Stat).to receive(:dossiers_states).and_return(stats) + allow(Stat).to receive(:deleted_dossiers_states).and_return(stats) + + Stat.update_stats + computed_stats = Stat.first + + expect(computed_stats.dossiers_not_brouillon).to eq(stats["not_brouillon"] * 2) + expect(computed_stats.dossiers_depose_avant_30_jours).to eq(stats["dossiers_depose_avant_30_jours"] * 2) + expect(computed_stats.dossiers_deposes_entre_60_et_30_jours).to eq(stats["dossiers_deposes_entre_60_et_30_jours"] * 2) + expect(computed_stats.dossiers_brouillon).to eq(stats["brouillon"] * 2) + expect(computed_stats.dossiers_en_construction).to eq(stats["en_construction"] * 2) + expect(computed_stats.dossiers_en_instruction).to eq(stats["en_instruction"] * 2) + expect(computed_stats.dossiers_termines).to eq(stats["termines"] * 2) + end + end + + describe '.cumulative_hash' do + it 'works count and cumulate counters by month for both dossier and deleted dossiers' do + 12.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(:cumulative_hash, [ + [Dossier.state_not_brouillon, :en_construction_at], + [DeletedDossier.where.not(state: :brouillon), :deleted_at] + ]) + expect(rs).to eq({ + 12.months.ago.utc.beginning_of_month => 2, + 11.months.ago.utc.beginning_of_month => 4, + 10.months.ago.utc.beginning_of_month => 6, + 9.months.ago.utc.beginning_of_month => 8, + 8.months.ago.utc.beginning_of_month => 10, + 7.months.ago.utc.beginning_of_month => 12, + 6.months.ago.utc.beginning_of_month => 14, + 5.months.ago.utc.beginning_of_month => 16, + 4.months.ago.utc.beginning_of_month => 18, + 3.months.ago.utc.beginning_of_month => 20, + 2.months.ago.utc.beginning_of_month => 22, + 1.month.ago.utc.beginning_of_month => 24 + }) + end + end + + 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) + 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 + + describe '.sum_hashes' do + it 'sum up hashes keys' do + expect(Stat.send(:sum_hashes, *[{ a: 1, b: 2, d: 5 }, { a: 2, b: 3, c: 5 }])).to eq({ a: 3, b: 5, c: 5, d: 5 }) + end + end +end