Merge pull request #7015 from betagouv/main

2022-03-08-01

app/assets/stylesheets/auth.scss

@@ -18,7 +18,15 @@
   }
 
   .column {
-    padding-top: 2 * $default-spacer;
+    padding-top: $default-spacer;
+  }
+
+  h1 {
+    margin-bottom: $default-spacer;
+  }
+
+  .form label {
+    margin-bottom: $default-spacer / 2;
   }
 }
 
@@ -26,7 +34,7 @@
   .auth-options {
     display: flex;
     justify-content: space-between;
-    margin-bottom: 4 * $default-spacer;
+    margin-bottom: 2 * $default-spacer;
   }
 
   .remember-me {
@@ -60,7 +68,7 @@
 
 .sign-in-form .form {
   input[type="email"] {
-    margin-bottom: $default-padding;
+    margin-bottom: $default-spacer;
   }
 
   input[type="password"] {

app/assets/stylesheets/france-connect-agent-login.scss

@@ -2,10 +2,14 @@
 @import "constants";
 
 .france-connect-agent-login-button {
-  background-image: image-url("logo-agent-connect.png");
+  background-image: image-url("agentconnect-btn-principal.svg"), image-url("agentconnect-btn-principal-hover.svg");
   display: block;
   height: 60px;
-  width: 230px;
+  width: 206px;
   margin: 20px auto;
   font-size: 0;
+
+  &:hover {
+    background-image: image-url("agentconnect-btn-principal-hover.svg");
+  }
 }

app/assets/stylesheets/france-connect-login.scss

@@ -19,14 +19,14 @@
   width: 230px;
   margin: auto;
   margin-bottom: 8px;
-  background-image: image-url("login-with-fc.svg"), image-url("login-with-fc-hover.svg");
+  background-image: image-url("franceconnect-btn.svg"), image-url("franceconnect-btn-hover.svg");
   background-repeat: no-repeat;
   background-size: cover;
   cursor: pointer;
   font-size: 0;
 
   &:hover {
-    background-image: image-url("login-with-fc-hover.svg");
+    background-image: image-url("franceconnect-btn-hover.svg");
   }
 }
 
@@ -36,8 +36,8 @@
   align-items: center;
   color: $black;
   text-transform: uppercase;
-  padding-bottom: $default-padding;
-  padding-top: $default-padding;
+  padding-bottom: $default-spacer;
+  padding-top: $default-spacer;
 
   &::before,
   &::after {

app/lib/database/migration_helpers.rb

@@ -1,4 +1,4 @@
-# Some of this file is lifted from Gitlab's `lib/gitlab/database/migration_helpers.rb``
+# Some of this file is lifted from Gitlab's `lib/gitlab/database/migration_helpers.rb`
 
 # Copyright (c) 2011-present GitLab B.V.
 #
@@ -103,6 +103,34 @@ module Database::MigrationHelpers
     end
   end
 
+  # Delete records from `from_table` having a reference to a missing record in `to_table`.
+  # This is useful to rectify data before adding a proper foreign_key.
+  #
+  # Example:
+  #
+  #     delete_orphans :appointments, :physicians
+  #
+  def delete_orphans(from_table, to_table)
+    say_with_time "Deleting records from #{from_table} where the associated #{to_table.to_s.singularize} no longer exists" do
+      from_table = Arel::Table.new(from_table)
+      to_table = Arel::Table.new(to_table)
+      foreign_key_column = foreign_key_column_for(to_table.name)
+
+      # Select the ids of orphan records
+      arel_select = from_table
+        .join(to_table, Arel::Nodes::OuterJoin).on(to_table[:id].eq(from_table[foreign_key_column]))
+        .where(to_table[:id].eq(nil))
+        .project(from_table[foreign_key_column])
+      missing_record_ids = query_values(arel_select.to_sql)
+
+      # Delete the records having ids referencing missing data
+      arel_delete = Arel::DeleteManager.new()
+        .from(from_table)
+        .where(from_table[foreign_key_column].in(missing_record_ids.uniq))
+      exec_delete(arel_delete.to_sql)
+    end
+  end
+
   private
 
   def statement_timeout_disabled?

app/models/procedure.rb

@@ -176,7 +176,7 @@ class Procedure < ApplicationRecord
     end
   end
 
-  has_many :administrateurs_procedures
+  has_many :administrateurs_procedures, dependent: :delete_all
   has_many :administrateurs, through: :administrateurs_procedures, after_remove: -> (procedure, _admin) { procedure.validate! }
   has_many :groupe_instructeurs, dependent: :destroy
   has_many :instructeurs, through: :groupe_instructeurs

app/views/administrateurs/dossier_submitted_messages/_informations.html.haml

@@ -1,3 +1,3 @@
 = f.label :message_on_submit_by_usager do
-  Message affiché après l'envoie du dossier
-= f.text_area :message_on_submit_by_usager, placeholder: "Merci votre dossier sera traité dans les plus bref delais"
+  Message affiché après l'envoi du dossier
+= f.text_area :message_on_submit_by_usager, placeholder: "Merci, votre dossier sera traité dans les plus bref delais"

app/views/administrateurs/procedures/show.html.haml

@@ -246,6 +246,6 @@
           %p.card-admin-status-todo À configurer
       %div
         %p.card-admin-title Fin de dépot
-        %p.card-admin-subtitle Orienter l'usager suite à l'envoie de son dossier
+        %p.card-admin-subtitle Orienter l'usager suite à l'envoi de son dossier
       %p.button Modifier
 

app/views/users/sessions/new.html.haml

@@ -27,6 +27,5 @@
     .france-connect-login-separator
       = t('views.shared.france_connect_login.separator')
     .center
-      %h2.important-header= t('views.users.sessions.new.state_civil_servant')
-      %br
+      %h2.important-header.mb-1= t('views.users.sessions.new.state_civil_servant')
       = link_to t('views.users.sessions.new.connect_with_agent_connect'), agent_connect_path, class: "button expend secondary"

config/locales/views/agent_connect/agent/en.yml

@@ -11,7 +11,7 @@ en:
             The ministries and operators that can currently benefit from it are :
           </p>
           <ul>
-            <li>the Ministry of Ecological Transition</li>
+            <li>the Insee</li>
           </ul>
         you_are_a_citizen: You are an individual ?
         citizen_page: Go to our dedicated page

config/locales/views/agent_connect/agent/fr.yml

@@ -8,10 +8,10 @@ fr:
           <p>
             <b class="bold">AgentConnect est en cours de déploiement.</b>
             <br>
-            Les ministères et opérateurs qui peuvent l'utiliser à ce jour sont&nbsp;:
+            Les ministères et opérateurs qui peuvent lʼutiliser à ce jour sont&nbsp;:
           </p>
           <ul>
-            <li>le ministère de la Transition écologique</li>
+            <li>lʼInsee</li>
           </ul>
         you_are_a_citizen: Vous êtes un particulier ?
         citizen_page: Accéder à notre page dédiée

config/locales/views/shared/en.yml

@@ -11,7 +11,7 @@ en:
         en_attente: "Waiting for response"
       france_connect_login:
         title: "With FranceConnect"
-        description: "France connect is a solution proposed by the government to secure and simplify the connection to web services."
+        description: "FranceConnect is a solution proposed by the government to secure and simplify the connection to web services."
         login_button: "Sign in with FranceConnect"
         help_link: What is FranceConnect?
         separator: or

config/locales/views/shared/fr.yml

@@ -11,7 +11,7 @@ fr:
         en_attente: "En attente de réponse"
       france_connect_login:
         title: 'Avec FranceConnect'
-        description: "France connect est la solution proposée par l’État pour sécuriser et simplifier la connexion aux services en ligne."
+        description: "FranceConnect est la solution proposée par l’État pour sécuriser et simplifier la connexion aux services en ligne."
         login_button: "S’identifier avec FranceConnect"
         help_link: "Qu’est-ce que FranceConnect ?"
         separator: 'ou'

db/migrate/20220301160753_add_administrateur_foreign_key_to_administrateurs_procedure.rb

@@ -1,12 +1,9 @@
 class AddAdministrateurForeignKeyToAdministrateursProcedure < ActiveRecord::Migration[6.1]
-  def up
-    # Sanity check
-    say_with_time 'Removing AdministrateursProcedures where the associated Administrateur no longer exists ' do
-      deleted_administrateur_ids = AdministrateursProcedure.where.missing(:administrateur).pluck(:administrateur_id)
-      AdministrateursProcedure.where(administrateur_id: deleted_administrateur_ids).delete_all
-    end
+  include Database::MigrationHelpers
 
-    add_foreign_key :administrateurs_procedures, :administrateurs
+  def up
+    delete_orphans :administrateurs_procedures, :administrateurs_procedures
+    add_foreign_key :administrateurs_procedures, :administrateurs_procedures
   end
 
   def down

db/migrate/20220302101337_add_foreign_keys_to_administrateurs_instructeurs.rb

@@ -1,17 +1,11 @@
 class AddForeignKeysToAdministrateursInstructeurs < ActiveRecord::Migration[6.1]
+  include Database::MigrationHelpers
+
   def up
-    # Sanity check
-    say_with_time 'Removing AdministrateursInstructeur where the associated Administrateur no longer exists ' do
-      deleted_administrateurs_ids = AdministrateursInstructeur.where.missing(:administrateur).pluck(:administrateur_id)
-      AdministrateursInstructeur.where(administrateur_id: deleted_administrateurs_ids).delete_all
-    end
-
-    say_with_time 'Removing AdministrateursInstructeur where the associated Instructeur no longer exists ' do
-      deleted_instructeurs_ids = AdministrateursInstructeur.where.missing(:instructeur).pluck(:instructeur_id)
-      AdministrateursInstructeur.where(instructeur_id: deleted_instructeurs_ids).delete_all
-    end
-
+    delete_orphans :administrateurs_instructeurs, :administrateurs
     add_foreign_key :administrateurs_instructeurs, :administrateurs
+
+    delete_orphans :administrateurs_instructeurs, :instructeurs
     add_foreign_key :administrateurs_instructeurs, :instructeurs
   end
 

db/migrate/20220308110720_add_procedure_foreign_key_to_administrateurs_procedure.rb

@@ -0,0 +1,12 @@
+class AddProcedureForeignKeyToAdministrateursProcedure < ActiveRecord::Migration[6.1]
+  include Database::MigrationHelpers
+
+  def up
+    delete_orphans :administrateurs_procedures, :procedures
+    add_foreign_key :administrateurs_procedures, :procedures
+  end
+
+  def down
+    remove_foreign_key :administrateurs_procedures, :procedures
+  end
+end

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: 2022_03_02_101337) do
+ActiveRecord::Schema.define(version: 2022_03_08_110720) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -855,6 +855,7 @@ ActiveRecord::Schema.define(version: 2022_03_02_101337) do
   add_foreign_key "administrateurs_instructeurs", "administrateurs"
   add_foreign_key "administrateurs_instructeurs", "instructeurs"
   add_foreign_key "administrateurs_procedures", "administrateurs"
+  add_foreign_key "administrateurs_procedures", "procedures"
   add_foreign_key "archives_groupe_instructeurs", "archives"
   add_foreign_key "archives_groupe_instructeurs", "groupe_instructeurs"
   add_foreign_key "assign_tos", "groupe_instructeurs"

spec/lib/database/migration_helpers_spec.rb

@@ -1,90 +1,194 @@
 describe Database::MigrationHelpers do
-  class TestLabel < ApplicationRecord
-  end
+  describe 'handling duplicates' do
+    class TestLabel < ApplicationRecord
+    end
 
-  before(:all) do
-    ActiveRecord::Migration.suppress_messages do
-      ActiveRecord::Migration.create_table "test_labels", force: true do |t|
-        t.string :label
-        t.integer :user_id
+    before(:all) do
+      ActiveRecord::Migration.suppress_messages do
+        ActiveRecord::Migration.create_table "test_labels", force: true do |t|
+          t.string :label
+          t.integer :user_id
+        end
+        ActiveRecord::Migration.create_table "test_labels", force: true do |t|
+          t.string :label
+          t.integer :user_id
+        end
       end
     end
-  end
 
-  before(:each) do
-    # User 1 labels
-    TestLabel.create({ id: 1, label: 'Important', user_id: 1 })
-    TestLabel.create({ id: 2, label: 'Urgent', user_id: 1 })
-    TestLabel.create({ id: 3, label: 'Done', user_id: 1 })
-    TestLabel.create({ id: 4, label: 'Bug', user_id: 1 })
+    before(:each) do
+      # User 1 labels
+      TestLabel.create({ id: 1, label: 'Important', user_id: 1 })
+      TestLabel.create({ id: 2, label: 'Urgent', user_id: 1 })
+      TestLabel.create({ id: 3, label: 'Done', user_id: 1 })
+      TestLabel.create({ id: 4, label: 'Bug', user_id: 1 })
 
-    # User 2 labels
-    TestLabel.create({ id: 5, label: 'Important', user_id: 2 })
-    TestLabel.create({ id: 6, label: 'Critical', user_id: 2 })
+      # User 2 labels
+      TestLabel.create({ id: 5, label: 'Important', user_id: 2 })
+      TestLabel.create({ id: 6, label: 'Critical', user_id: 2 })
 
-    # Duplicates
-    TestLabel.create({ id: 7, label: 'Urgent', user_id: 1 })
-    TestLabel.create({ id: 8, label: 'Important', user_id: 2 })
-  end
-
-  after(:all) do
-    ActiveRecord::Migration.suppress_messages do
-      ActiveRecord::Migration.drop_table :test_labels, force: true
+      # Duplicates
+      TestLabel.create({ id: 7, label: 'Urgent', user_id: 1 })
+      TestLabel.create({ id: 8, label: 'Important', user_id: 2 })
     end
-  end
 
-  let(:model) { ActiveRecord::Migration.new.extend(Database::MigrationHelpers) }
+    after(:all) do
+      ActiveRecord::Migration.suppress_messages do
+        ActiveRecord::Migration.drop_table :test_labels, force: true
+      end
+    end
 
-  describe '.find_duplicates' do
-    context 'using a single column for uniqueness' do
+    let(:model) { ActiveRecord::Migration.new.extend(Database::MigrationHelpers) }
+
+    describe '.find_duplicates' do
+      context 'using a single column for uniqueness' do
+        subject do
+          model.find_duplicates(:test_labels, [:label])
+        end
+
+        it 'finds duplicates' do
+          expect(subject.length).to eq 2
+        end
+
+        it 'finds three labels with "Important"' do
+          expect(subject).to include [1, 5, 8]
+        end
+
+        it 'finds two labels with "Urgent"' do
+          expect(subject).to include [2, 7]
+        end
+      end
+
+      context 'using multiple columns for uniqueness' do
+        subject do
+          model.find_duplicates(:test_labels, [:label, :user_id])
+        end
+
+        it 'finds duplicates' do
+          expect(subject.length).to eq 2
+        end
+
+        it 'finds two labels with "Important" for user 2' do
+          expect(subject).to include [5, 8]
+        end
+
+        it 'finds two labels with "Urgent" for user 1' do
+          expect(subject).to include [2, 7]
+        end
+      end
+    end
+
+    describe '.delete_duplicates' do
       subject do
-        model.find_duplicates(:test_labels, [:label])
+        model.delete_duplicates(:test_labels, [:label])
       end
 
-      it 'finds duplicates' do
-        expect(subject.length).to eq 2
-      end
-
-      it 'finds three labels with "Important"' do
-        expect(subject).to include [1, 5, 8]
-      end
-
-      it 'finds two labels with "Urgent"' do
-        expect(subject).to include [2, 7]
-      end
-    end
-
-    context 'using multiple columns for uniqueness' do
-      subject do
-        model.find_duplicates(:test_labels, [:label, :user_id])
-      end
-
-      it 'finds duplicates' do
-        expect(subject.length).to eq 2
-      end
-
-      it 'finds two labels with "Important" for user 2' do
-        expect(subject).to include [5, 8]
-      end
-
-      it 'finds two labels with "Urgent" for user 1' do
-        expect(subject).to include [2, 7]
+      it 'keeps the first item, and delete the others' do
+        expect { subject }.to change(TestLabel, :count).by(-3)
+        expect(TestLabel.where(label: 'Critical').count).to eq(1)
+        expect(TestLabel.where(label: 'Important').count).to eq(1)
+        expect(TestLabel.where(label: 'Urgent').count).to eq(1)
+        expect(TestLabel.where(label: 'Bug').count).to eq(1)
+        expect(TestLabel.where(label: 'Done').count).to eq(1)
       end
     end
   end
 
-  describe '.delete_duplicates' do
+  describe '.delete_orphans' do
+    class TestPhysician < ApplicationRecord; end
+
+    class TestPatient < ApplicationRecord; end
+
+    class TestAppointment < ApplicationRecord; end
+
+    before(:all) do
+      ActiveRecord::Migration.suppress_messages do
+        ActiveRecord::Migration.create_table "test_physicians", force: true do |t|
+          t.string :name
+        end
+        ActiveRecord::Migration.create_table "test_patients", force: true do |t|
+          t.string :name
+        end
+        ActiveRecord::Migration.create_table "test_appointments", id: false, force: true do |t|
+          t.integer  :test_physician_id
+          t.integer  :test_patient_id
+          t.datetime :datetime
+        end
+      end
+    end
+
+    after(:all) do
+      ActiveRecord::Migration.suppress_messages do
+        ActiveRecord::Migration.drop_table :test_physicians, force: true
+        ActiveRecord::Migration.drop_table :test_patients, force: true
+        ActiveRecord::Migration.drop_table :test_appointments, force: true
+      end
+    end
+
+    let(:model) { ActiveRecord::Migration.new.extend(Database::MigrationHelpers) }
+
     subject do
-      model.delete_duplicates(:test_labels, [:label])
+      model.delete_orphans(:test_appointments, :test_patients)
     end
 
-    it 'keeps the first item, and delete the others' do
-      expect { subject }.to change(TestLabel, :count).by(-3)
-      expect(TestLabel.where(label: 'Critical').count).to eq(1)
-      expect(TestLabel.where(label: 'Important').count).to eq(1)
-      expect(TestLabel.where(label: 'Urgent').count).to eq(1)
-      expect(TestLabel.where(label: 'Bug').count).to eq(1)
-      expect(TestLabel.where(label: 'Done').count).to eq(1)
+    context 'when there are orphan records' do
+      before(:each) do
+        phy1 = TestPhysician.create({ name: 'Ibn Sina' })
+        phy2 = TestPhysician.create({ name: 'Louis Pasteur' })
+        pa1 = TestPatient.create({ name: 'Chams ad-Dawla' })
+        pa2 = TestPatient.create({ name: 'Joseph Meister' })
+        ap1 = TestAppointment.create({ test_physician_id: phy1.id, test_patient_id: pa1.id, datetime: 2.months.ago })
+        ap2 = TestAppointment.create({ test_physician_id: phy1.id, test_patient_id: pa1.id, datetime: 1.month.ago })
+        ap3 = TestAppointment.create({ test_physician_id: phy2.id, test_patient_id: pa2.id, datetime: 2.days.ago })
+        ap4 = TestAppointment.create({ test_physician_id: phy1.id, test_patient_id: pa2.id, datetime: 1.day.ago })
+        ap5 = TestAppointment.create({ test_physician_id: phy1.id, test_patient_id: pa1.id, datetime: Time.zone.today })
+
+        # Appointments missing the associated patient
+        ap6 = TestAppointment.create({ test_physician_id: phy1.id, test_patient_id: 9999, datetime: 3.months.ago })
+        ap7 = TestAppointment.create({ test_physician_id: phy1.id, test_patient_id: 8888, datetime: 2.months.ago })
+        ap8 = TestAppointment.create({ test_physician_id: phy2.id, test_patient_id: 8888, datetime: 1.month.ago })
+
+        # Appointments missing the associated physician
+        ap9 = TestAppointment.create({ test_physician_id: 7777, test_patient_id: pa1.id, datetime: 3.months.ago })
+      end
+
+      it 'deletes orphaned records on the specified key' do
+        expect { subject }.to change { TestAppointment.count }.by(-3)
+
+        # rubocop:disable Rails/WhereEquals
+        appointments_with_missing_patients = TestAppointment
+          .joins('LEFT OUTER JOIN test_patients ON test_patients.id = test_appointments.test_patient_id')
+          .where('test_patients.id IS NULL')
+        # rubocop:enable Rails/WhereEquals
+        expect(appointments_with_missing_patients.count).to eq(0)
+      end
+
+      it 'keeps orphaned records on another key' do
+        subject
+
+        # rubocop:disable Rails/WhereEquals
+        appointments_with_missing_physicians = TestAppointment
+          .joins('LEFT OUTER JOIN test_physicians ON test_physicians.id = test_appointments.test_physician_id')
+          .where('test_physicians.id IS NULL')
+        # rubocop:enable Rails/WhereEquals
+        expect(appointments_with_missing_physicians.count).not_to eq(0)
+      end
+
+      it 'keeps valid associated records' do
+        expect { subject }.not_to change { [TestPhysician.count, TestPatient.count] }
+      end
+    end
+
+    context 'when there are no orphaned records' do
+      before(:each) do
+        phy1 = TestPhysician.create({ name: 'Ibn Sina' })
+        pa1 = TestPatient.create({ name: 'Chams ad-Dawla' })
+        ap1 = TestAppointment.create({ test_physician_id: phy1.id, test_patient_id: pa1.id, datetime: 2.months.ago })
+      end
+
+      it 'doesn’t remove any records' do
+        expect { subject }.not_to change { [TestPhysician.count, TestPatient.count, TestAppointment.count] }
+      end
     end
   end