From befee0ea57db6ea72ed0b25d52205c0509d9efc8 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 30 Jul 2019 16:02:02 +0000 Subject: [PATCH 1/5] Drop `types_de_piece_justificative` and `pieces_justificatives` tables Now that the content of these tables has been migrated, we can drop them. --- ...p_deprecated_piece_justificative_tables.rb | 40 +++++++++++++++++++ db/schema.rb | 28 +------------ 2 files changed, 41 insertions(+), 27 deletions(-) create mode 100644 db/migrate/20190730153555_drop_deprecated_piece_justificative_tables.rb diff --git a/db/migrate/20190730153555_drop_deprecated_piece_justificative_tables.rb b/db/migrate/20190730153555_drop_deprecated_piece_justificative_tables.rb new file mode 100644 index 000000000..8a3b6c53c --- /dev/null +++ b/db/migrate/20190730153555_drop_deprecated_piece_justificative_tables.rb @@ -0,0 +1,40 @@ +class DropDeprecatedPieceJustificativeTables < ActiveRecord::Migration[5.2] + def change + assert_empty_table!(:types_de_piece_justificative) + assert_empty_table!(:pieces_justificatives) + + drop_table :types_de_piece_justificative do |t| + t.string "libelle" + t.string "description" + t.boolean "api_entreprise", default: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.integer "procedure_id" + t.integer "order_place" + t.string "lien_demarche" + t.boolean "mandatory", default: false + t.index ["procedure_id"], name: "index_types_de_piece_justificative_on_procedure_id" + end + + drop_table :pieces_justificatives do |t| + t.string "content" + t.integer "dossier_id" + t.integer "type_de_piece_justificative_id" + t.datetime "created_at" + t.integer "user_id" + t.string "original_filename" + t.string "content_secure_token" + t.datetime "updated_at" + t.index ["dossier_id"], name: "index_pieces_justificatives_on_dossier_id" + t.index ["type_de_piece_justificative_id"], name: "index_pieces_justificatives_on_type_de_piece_justificative_id" + end + end + + def assert_empty_table!(table) + results = ActiveRecord::Base.connection.exec_query("SELECT COUNT(*) FROM #{table}") + records_count = results.first['count'] + if records_count > 0 + raise "Abord dropping `#{table}` table: it still contains #{records_count} records." + end + end +end diff --git a/db/schema.rb b/db/schema.rb index a93ebee34..5db9056d5 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: 2019_07_17_151228) do +ActiveRecord::Schema.define(version: 2019_07_30_153555) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -456,19 +456,6 @@ ActiveRecord::Schema.define(version: 2019_07_17_151228) do t.index ["procedure_id"], name: "index_module_api_cartos_on_procedure_id", unique: true end - create_table "pieces_justificatives", id: :serial, force: :cascade do |t| - t.string "content" - t.integer "dossier_id" - t.integer "type_de_piece_justificative_id" - t.datetime "created_at" - t.integer "user_id" - t.string "original_filename" - t.string "content_secure_token" - t.datetime "updated_at" - t.index ["dossier_id"], name: "index_pieces_justificatives_on_dossier_id" - t.index ["type_de_piece_justificative_id"], name: "index_pieces_justificatives_on_type_de_piece_justificative_id" - end - create_table "procedure_presentations", id: :serial, force: :cascade do |t| t.integer "assign_to_id" t.jsonb "sort", default: {"order"=>"desc", "table"=>"notifications", "column"=>"notifications"}, null: false @@ -587,19 +574,6 @@ ActiveRecord::Schema.define(version: 2019_07_17_151228) do t.index ["stable_id"], name: "index_types_de_champ_on_stable_id" end - create_table "types_de_piece_justificative", id: :serial, force: :cascade do |t| - t.string "libelle" - t.string "description" - t.boolean "api_entreprise", default: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.integer "procedure_id" - t.integer "order_place" - t.string "lien_demarche" - t.boolean "mandatory", default: false - t.index ["procedure_id"], name: "index_types_de_piece_justificative_on_procedure_id" - end - create_table "users", id: :serial, force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false From 20239077a75e9810f04887c2f979696775f53181 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 1 Aug 2019 15:00:23 +0200 Subject: [PATCH 2/5] Gemfile: fix an 'insecure connection' Bundler warning --- Gemfile | 2 +- Gemfile.lock | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Gemfile b/Gemfile index 4bc7bd8ca..020990ae5 100644 --- a/Gemfile +++ b/Gemfile @@ -1,7 +1,7 @@ source 'https://rubygems.org' gem 'aasm' -gem 'actiontext', github: 'kobaltz/actiontext', branch: 'archive', require: 'action_text' +gem 'actiontext', git: 'https://github.com/kobaltz/actiontext.git', branch: 'archive', require: 'action_text' # Port of ActionText to Rails 5 gem 'active_link_to' # Automatically set a class on active links gem 'active_model_serializers' gem 'activestorage-openstack', git: 'https://github.com/fredZen/activestorage-openstack.git', branch: 'frederic/fix_upload_signature' diff --git a/Gemfile.lock b/Gemfile.lock index 8b597a428..055c310e9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,12 +1,3 @@ -GIT - remote: git://github.com/kobaltz/actiontext.git - revision: ef59c4ba99d1b7614dd47f5a294eef553224db88 - branch: archive - specs: - actiontext (0.1.0) - nokogiri - rails (>= 5.2.0) - GIT remote: https://github.com/fredZen/activestorage-openstack.git revision: c71d5107a51701eab9d9267dd0000e6c1cf3e39a @@ -18,6 +9,15 @@ GIT mime-types rails (~> 5.2.0) +GIT + remote: https://github.com/kobaltz/actiontext.git + revision: ef59c4ba99d1b7614dd47f5a294eef553224db88 + branch: archive + specs: + actiontext (0.1.0) + nokogiri + rails (>= 5.2.0) + GIT remote: https://github.com/mina-deploy/mina.git revision: 0dd5fdb8bb82a180d35e1fc033de2fac48257e30 From 6459e9cf37164d4ba9b308f1fcb525daee515ce2 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 1 Aug 2019 12:46:47 +0000 Subject: [PATCH 3/5] tasks: fix the commentaires migration task to work with hidden dossiers By default `commentaire.dossier` doesn't return the dossier if it is hidden. --- .../2019_05_29_migrate_commentaire_pj.rake | 5 ++-- spec/factories/dossier.rb | 4 +++ .../2019_05_29_migrate_commentaire_pj_spec.rb | 30 +++++++++++++++---- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/lib/tasks/2019_05_29_migrate_commentaire_pj.rake b/lib/tasks/2019_05_29_migrate_commentaire_pj.rake index 1725d40c0..60cb5d668 100644 --- a/lib/tasks/2019_05_29_migrate_commentaire_pj.rake +++ b/lib/tasks/2019_05_29_migrate_commentaire_pj.rake @@ -14,12 +14,13 @@ namespace :'2019_05_29_migrate_commentaire_pj' do progress = ProgressReport.new(commentaires.count) commentaires.find_each do |commentaire| if commentaire.file.present? + dossier = Dossier.unscope(where: :hidden_at).find(commentaire.dossier_id) uri = URI.parse(URI.escape(commentaire.file_url)) response = Typhoeus.get(uri) if response.success? filename = commentaire.file.filename || commentaire.file_identifier updated_at = commentaire.updated_at - dossier_updated_at = commentaire.dossier.updated_at + dossier_updated_at = dossier.updated_at commentaire.piece_jointe.attach( io: StringIO.new(response.body), filename: filename, @@ -27,7 +28,7 @@ namespace :'2019_05_29_migrate_commentaire_pj' do metadata: { virus_scan_result: ActiveStorage::VirusScanner::SAFE } ) commentaire.update_column(:updated_at, updated_at) - commentaire.dossier.update_column(:updated_at, dossier_updated_at) + dossier.update_column(:updated_at, dossier_updated_at) end end progress.inc diff --git a/spec/factories/dossier.rb b/spec/factories/dossier.rb index 350274458..a138f3842 100644 --- a/spec/factories/dossier.rb +++ b/spec/factories/dossier.rb @@ -39,6 +39,10 @@ FactoryBot.define do archived { false } end + trait :hidden do + hidden_at { Time.zone.now } + end + trait :with_dossier_link do after(:create) do |dossier, _evaluator| linked_dossier = create(:dossier) diff --git a/spec/lib/tasks/2019_05_29_migrate_commentaire_pj_spec.rb b/spec/lib/tasks/2019_05_29_migrate_commentaire_pj_spec.rb index 7df801b37..b05875879 100644 --- a/spec/lib/tasks/2019_05_29_migrate_commentaire_pj_spec.rb +++ b/spec/lib/tasks/2019_05_29_migrate_commentaire_pj_spec.rb @@ -1,14 +1,16 @@ describe '2019_05_29_migrate_commentaire_pj.rake' do let(:rake_task) { Rake::Task['2019_05_29_migrate_commentaire_pj:run'] } - let!(:commentaires) do - create(:commentaire) - create(:commentaire, :with_file) - create(:commentaire, :with_file) + let(:commentaires) do + [ + create(:commentaire), + create(:commentaire, :with_file), + create(:commentaire, :with_file) + ] end before do - Commentaire.all.each do |commentaire| + commentaires.each do |commentaire| if commentaire.file.present? stub_request(:get, commentaire.file_url) .to_return(status: 200, body: File.read(commentaire.file.path)) @@ -39,4 +41,22 @@ describe '2019_05_29_migrate_commentaire_pj.rake' do expect(Commentaire.where(file: nil).count).to eq(1) expect(Commentaire.all.map(&:piece_jointe).map(&:attached?)).to eq([false, true, false]) end + + context 'when a commentaire’s dossier is hidden' do + let(:hidden_dossier) { create(:dossier, :en_construction, :hidden) } + let(:commentaire) { create(:commentaire, :with_file, dossier: hidden_dossier) } + let(:commentaires) { [commentaire] } + + it 'should migrate the pj' do + comment_updated_at = commentaire.reload.updated_at + dossier_updated_at = hidden_dossier.reload.updated_at + + rake_task.invoke + commentaires.each(&:reload) + + expect(commentaire.piece_jointe.attached?).to be true + expect(commentaire.updated_at).to eq(comment_updated_at) + expect(hidden_dossier.updated_at).to eq(dossier_updated_at) + end + end end From b8d3e4c41b44d22010e53ecffe034e78a380db43 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 1 Aug 2019 14:07:35 +0200 Subject: [PATCH 4/5] Fix attestation url for dossiers with no attestation --- app/serializers/dossier_serializer.rb | 2 +- .../api/v1/dossiers_controller_spec.rb | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/app/serializers/dossier_serializer.rb b/app/serializers/dossier_serializer.rb index 358a88a21..ae62f6eb9 100644 --- a/app/serializers/dossier_serializer.rb +++ b/app/serializers/dossier_serializer.rb @@ -58,7 +58,7 @@ class DossierSerializer < ActiveModel::Serializer end def attestation - object.attestation.pdf_url + object.attestation&.pdf_url end def justificatif_motivation diff --git a/spec/controllers/api/v1/dossiers_controller_spec.rb b/spec/controllers/api/v1/dossiers_controller_spec.rb index 955e2dac5..ad9907b93 100644 --- a/spec/controllers/api/v1/dossiers_controller_spec.rb +++ b/spec/controllers/api/v1/dossiers_controller_spec.rb @@ -151,10 +151,25 @@ describe API::V1::DossiersController do it { expect(subject.code).to eq('404') } end + context 'when dossier (with attestation) exists and belongs to procedure' do + let(:procedure_id) { procedure.id } + let(:dossier_id) { dossier.id } + let!(:dossier) { create(:dossier, :with_entreprise, :with_attestation, :accepte, procedure: procedure, motivation: "Motivation") } + let(:body) { JSON.parse(retour.body, symbolize_names: true) } + subject { body[:dossier] } + + it { + expect(retour.code).to eq('200') + expect(subject[:id]).to eq(dossier.id) + expect(subject[:state]).to eq('closed') + expect(subject[:attestation]).to_not be_nil + } + end + context 'when dossier exists and belongs to procedure' do let(:procedure_id) { procedure.id } let(:date_creation) { Time.zone.local(2008, 9, 1, 10, 5, 0) } - let!(:dossier) { Timecop.freeze(date_creation) { create(:dossier, :with_entreprise, :with_attestation, :accepte, procedure: procedure, motivation: "Motivation") } } + let!(:dossier) { Timecop.freeze(date_creation) { create(:dossier, :with_entreprise, :accepte, procedure: procedure, motivation: "Motivation") } } let(:dossier_id) { dossier.id } let(:body) { JSON.parse(retour.body, symbolize_names: true) } let(:field_list) { [:id, :created_at, :updated_at, :archived, :individual, :entreprise, :etablissement, :cerfa, :types_de_piece_justificative, :pieces_justificatives, :champs, :champs_private, :commentaires, :state, :simplified_state, :initiated_at, :processed_at, :received_at, :motivation, :email, :instructeurs, :attestation, :avis] } From 02904f55b1a1a602e8e229111e6cf03800ff5571 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 1 Aug 2019 15:48:27 +0200 Subject: [PATCH 5/5] Fix champ policy --- app/controllers/application_controller.rb | 12 +++---- app/models/gestionnaire.rb | 4 --- app/policies/application_policy.rb | 8 +++-- app/policies/champ_policy.rb | 13 +------- app/policies/type_de_champ_policy.rb | 4 +-- spec/policies/champ_policy_spec.rb | 39 +++------------------- spec/policies/type_de_champ_policy_spec.rb | 15 +++++++-- 7 files changed, 29 insertions(+), 66 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f405328a9..702cb9acb 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -45,13 +45,11 @@ class ApplicationController < ActionController::Base helper_method :logged_in? def pundit_user - if administrateur_signed_in? - current_administrateur - elsif gestionnaire_signed_in? - current_gestionnaire - else - current_user - end + { + administrateur: current_administrateur, + gestionnaire: current_gestionnaire, + user: current_user + }.compact end protected diff --git a/app/models/gestionnaire.rb b/app/models/gestionnaire.rb index ea633c7e8..1c94f5070 100644 --- a/app/models/gestionnaire.rb +++ b/app/models/gestionnaire.rb @@ -225,10 +225,6 @@ class Gestionnaire < ApplicationRecord end end - def user - User.find_by(email: email) - end - private def annotations_hash(demande, annotations_privees, avis, messagerie) diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index eefe976c6..e3bdc3791 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -35,10 +35,12 @@ class ApplicationPolicy end class Scope - attr_reader :user, :scope + attr_reader :user, :gestionnaire, :administrateur, :scope - def initialize(user, scope) - @user = user + def initialize(account, scope) + @user = account[:user] + @gestionnaire = account[:gestionnaire] + @administrateur = account[:administrateur] @scope = scope end diff --git a/app/policies/champ_policy.rb b/app/policies/champ_policy.rb index 601fa4529..26bd9a01a 100644 --- a/app/policies/champ_policy.rb +++ b/app/policies/champ_policy.rb @@ -1,21 +1,10 @@ class ChampPolicy < ApplicationPolicy class Scope < Scope def resolve - if user.is_a?(User) + if user.present? scope .joins(:dossier) .where({ dossiers: { user_id: user.id } }) - elsif user.is_a?(Gestionnaire) - scope_with_join = scope.joins(dossier: :follows) - scope_with_left_join = scope.left_joins(dossier: :follows) - - if user.user - scope_with_left_join - .where({ dossiers: { user_id: user.user.id } }) - .or(scope_with_left_join.where(dossiers: { follows: { gestionnaire_id: user.id } })) - else - scope_with_join.where(dossiers: { follows: { gestionnaire_id: user.id } }) - end else scope.none end diff --git a/app/policies/type_de_champ_policy.rb b/app/policies/type_de_champ_policy.rb index 3fab51906..3f8390014 100644 --- a/app/policies/type_de_champ_policy.rb +++ b/app/policies/type_de_champ_policy.rb @@ -1,10 +1,10 @@ class TypeDeChampPolicy < ApplicationPolicy class Scope < Scope def resolve - if user.is_a?(Administrateur) + if administrateur.present? scope .joins(procedure: [:administrateurs]) - .where({ administrateurs: { id: user.id } }) + .where({ administrateurs: { id: administrateur.id } }) else scope.none end diff --git a/spec/policies/champ_policy_spec.rb b/spec/policies/champ_policy_spec.rb index 2e06f464c..4dec03f29 100644 --- a/spec/policies/champ_policy_spec.rb +++ b/spec/policies/champ_policy_spec.rb @@ -5,12 +5,13 @@ describe ChampPolicy do let(:dossier) { create(:dossier, user: user) } let!(:champ) { create(:champ_text, dossier: dossier) } - let(:pundit_user) { user } - subject { Pundit.policy_scope(pundit_user, Champ) } + let(:account) { { user: user } } + + subject { Pundit.policy_scope(account, Champ) } context 'when the user has only user rights' do context 'cannot access champs for other dossiers' do - let(:pundit_user) { create(:user) } + let(:account) { { user: create(:user) } } it { expect(subject.find_by(id: champ.id)).to eq(nil) } end @@ -21,36 +22,4 @@ describe ChampPolicy do } end end - - context 'when the user has only gestionnaire rights' do - context 'can access champs for dossiers it follows' do - let(:dossier) { create(:dossier, :followed) } - let(:pundit_user) { dossier.followers_gestionnaires.first } - - it { expect(subject.find(champ.id)).to eq(champ) } - end - end - - context 'when the user has user and gestionnaire rights' do - let(:pundit_user) { dossier.followers_gestionnaires.first } - let(:dossier) { create(:dossier, :followed) } - - let(:user) { create(:user, email: pundit_user.email) } - let(:dossier2) { create(:dossier, user: user) } - let!(:champ_2) { create(:champ_text, dossier: dossier2) } - - context 'can access champs for dossiers it follows' do - it do - expect(pundit_user.user).to eq(user) - expect(subject.find(champ.id)).to eq(champ) - end - end - - context 'can access champs for its own dossiers' do - it do - expect(pundit_user.user).to eq(user) - expect(subject.find(champ_2.id)).to eq(champ_2) - end - end - end end diff --git a/spec/policies/type_de_champ_policy_spec.rb b/spec/policies/type_de_champ_policy_spec.rb index ca3e7e635..e16491f37 100644 --- a/spec/policies/type_de_champ_policy_spec.rb +++ b/spec/policies/type_de_champ_policy_spec.rb @@ -4,8 +4,17 @@ describe TypeDeChampPolicy do let(:procedure) { create(:procedure) } let!(:type_de_champ) { create(:type_de_champ_text, procedure: procedure) } - let(:pundit_user) { create(:user) } - subject { Pundit.policy_scope(pundit_user, TypeDeChamp) } + let(:user) { create(:user) } + let(:administrateur) { nil } + + let(:account) do + { + user: user, + administrateur: administrateur + }.compact + end + + subject { Pundit.policy_scope(account, TypeDeChamp) } context 'when the user has only user rights' do it 'can not access' do @@ -14,7 +23,7 @@ describe TypeDeChampPolicy do end context 'when the user has administrateur rights' do - let(:pundit_user) { procedure.administrateurs.first } + let(:administrateur) { procedure.administrateurs.first } it 'can access' do expect(subject.find(type_de_champ.id)).to eq(type_de_champ)