Merge pull request #5439 from betagouv/5288-update-dashboard

5288 - Update dashboard to improve performance

app/models/dossier.rb

@@ -288,7 +288,27 @@ class Dossier < ApplicationRecord
   scope :for_procedure, -> (procedure) { includes(:user, :groupe_instructeur).where(groupe_instructeurs: { procedure: procedure }) }
   scope :for_api_v2, -> { includes(procedure: [:administrateurs], etablissement: [], individual: [], traitements: []) }
 
-  scope :with_notifications, -> do
+  # todo: once we are sure with_cached_notifications does not introduce regressions, remove with_unoptimized_notifications
+  # and use with_cached_notifications instead
+  scope :with_notifications, -> (instructeur) do
+    if Flipper.enabled?(:cached_notifications, instructeur)
+      return with_cached_notifications
+    else
+      return with_unoptimized_notifications
+    end
+  end
+
+  scope :with_cached_notifications, -> do
+    joins(:follows)
+      .where('last_champ_updated_at > follows.demande_seen_at' \
+      ' OR groupe_instructeur_updated_at > follows.demande_seen_at' \
+      ' OR last_champ_private_updated_at > follows.annotations_privees_seen_at' \
+      ' OR last_avis_updated_at > follows.avis_seen_at' \
+      ' OR last_commentaire_updated_at > follows.messagerie_seen_at')
+      .distinct
+  end
+
+  scope :with_unoptimized_notifications, -> do
     # This scope is meant to be composed, typically with Instructeur.followed_dossiers, which means that the :follows table is already INNER JOINed;
     # it will fail otherwise
     # rubocop:disable DS/ApplicationName

app/models/instructeur.rb

@@ -141,14 +141,14 @@ class Instructeur < ApplicationRecord
       .where(groupe_instructeur: target_groupes)
       .send(scope) # :en_cours or :termine or :not_archived (or any other Dossier scope)
       .merge(followed_dossiers)
-      .with_notifications
+      .with_notifications(self)
   end
 
   def procedures_with_notifications(scope)
     dossiers = Dossier
       .send(scope) # :en_cours or :termine (or any other Dossier scope)
       .merge(followed_dossiers)
-      .with_notifications
+      .with_notifications(self)
 
     Procedure
       .where(id: dossiers.joins(:groupe_instructeur)
@@ -213,6 +213,10 @@ class Instructeur < ApplicationRecord
   def features
   end
 
+  def flipper_id
+    "Instructeur:#{id}"
+  end
+
   private
 
   def annotations_hash(demande, annotations_privees, avis, messagerie)

app/models/procedure_presentation.rb

@@ -91,7 +91,7 @@ class ProcedurePresentation < ApplicationRecord
 
     case table
     when 'notifications'
-      dossiers_id_with_notification = dossiers.merge(instructeur.followed_dossiers).with_notifications.ids
+      dossiers_id_with_notification = dossiers.merge(instructeur.followed_dossiers).with_notifications(instructeur).ids
       if order == 'desc'
         return dossiers_id_with_notification +
             (dossiers.order('dossiers.updated_at desc').ids - dossiers_id_with_notification)

spec/controllers/instructeurs/dossiers_controller_spec.rb

@@ -445,79 +445,64 @@ describe Instructeurs::DossiersController, type: :controller do
       }
     end
 
-    before do
-      subject
-    end
-
     let(:emails) { ['email@a.com'] }
 
-    it { expect(saved_avis.email).to eq('email@a.com') }
+    context "notifications updates" do
-    it { expect(saved_avis.introduction).to eq('intro') }
+      context 'when an instructeur follows the dossier' do
-    it { expect(saved_avis.confidentiel).to eq(true) }
+        let(:follower) { create(:instructeur) }
-    it { expect(saved_avis.dossier).to eq(dossier) }
+        before { follower.follow(dossier) }
-    it { expect(saved_avis.claimant).to eq(instructeur) }
-    it { expect(response).to redirect_to(avis_instructeur_dossier_path(dossier.procedure, dossier)) }
 
-    context "with an invalid email" do
+        it 'the follower has a notification' do
-      let(:emails) { ['emaila.com'] }
+          expect(follower.followed_dossiers.with_notifications(follower)).to eq([])
-
+          subject
-      it { expect(response).to render_template :avis }
+          expect(follower.followed_dossiers.with_notifications(follower)).to eq([dossier.reload])
-      it { expect(flash.alert).to eq(["emaila.com : Email n'est pas valide"]) }
-      it { expect { subject }.not_to change(Avis, :count) }
-      it { expect(dossier.last_avis_updated_at).to eq(nil) }
-    end
-
-    context 'with multiple emails' do
-      let(:emails) { ["toto.fr,titi@titimail.com"] }
-
-      it { expect(response).to render_template :avis }
-      it { expect(flash.alert).to eq(["toto.fr : Email n'est pas valide"]) }
-      it { expect(flash.notice).to eq("Une demande d'avis a été envoyée à titi@titimail.com") }
-      it { expect(Avis.count).to eq(old_avis_count + 1) }
-      it { expect(saved_avis.email).to eq("titi@titimail.com") }
-    end
-
-    context 'with linked dossiers' do
-      let(:asked_confidentiel) { false }
-      let(:previous_avis_confidentiel) { false }
-      let(:dossier) { create(:dossier, :en_construction, :with_dossier_link, procedure: procedure) }
-
-      context 'when the expert doesn’t share linked dossiers' do
-        let(:invite_linked_dossiers) { false }
-
-        it 'sends a single avis for the main dossier, but doesn’t give access to the linked dossiers' do
-          expect(flash.notice).to eq("Une demande d'avis a été envoyée à email@a.com")
-          expect(Avis.count).to eq(old_avis_count + 1)
-          expect(saved_avis.email).to eq("email@a.com")
-          expect(saved_avis.dossier).to eq(dossier)
         end
       end
+    end
 
-      context 'when the expert also shares the linked dossiers' do
+    context 'email sending' do
-        let(:invite_linked_dossiers) { true }
+      before do
+        subject
+      end
 
-        context 'and the expert can access the linked dossiers' do
+      it { expect(saved_avis.email).to eq('email@a.com') }
-          let(:saved_avis) { Avis.last(2).first }
+      it { expect(saved_avis.introduction).to eq('intro') }
-          let(:linked_avis) { Avis.last }
+      it { expect(saved_avis.confidentiel).to eq(true) }
-          let(:linked_dossier) { Dossier.find_by(id: dossier.reload.champs.filter(&:dossier_link?).map(&:value).compact) }
+      it { expect(saved_avis.dossier).to eq(dossier) }
-          let(:invite_linked_dossiers) do
+      it { expect(saved_avis.claimant).to eq(instructeur) }
-            instructeur.assign_to_procedure(linked_dossier.procedure)
+      it { expect(response).to redirect_to(avis_instructeur_dossier_path(dossier.procedure, dossier)) }
-            true
-          end
 
-          it 'sends one avis for the main dossier' do
+      context "with an invalid email" do
-            expect(flash.notice).to eq("Une demande d'avis a été envoyée à email@a.com")
+        let(:emails) { ['emaila.com'] }
-            expect(saved_avis.email).to eq("email@a.com")
-            expect(saved_avis.dossier).to eq(dossier)
-          end
 
-          it 'sends another avis for the linked dossiers' do
+        before { subject }
-            expect(Avis.count).to eq(old_avis_count + 2)
+
-            expect(linked_avis.dossier).to eq(linked_dossier)
+        it { expect(response).to render_template :avis }
-          end
+        it { expect(flash.alert).to eq(["emaila.com : Email n'est pas valide"]) }
-        end
+        it { expect { subject }.not_to change(Avis, :count) }
+        it { expect(dossier.last_avis_updated_at).to eq(nil) }
+      end
+
+      context 'with multiple emails' do
+        let(:emails) { ["toto.fr,titi@titimail.com"] }
+
+        before { subject }
+
+        it { expect(response).to render_template :avis }
+        it { expect(flash.alert).to eq(["toto.fr : Email n'est pas valide"]) }
+        it { expect(flash.notice).to eq("Une demande d'avis a été envoyée à titi@titimail.com") }
+        it { expect(Avis.count).to eq(old_avis_count + 1) }
+        it { expect(saved_avis.email).to eq("titi@titimail.com") }
+      end
+
+      context 'with linked dossiers' do
+        let(:asked_confidentiel) { false }
+        let(:previous_avis_confidentiel) { false }
+        let(:dossier) { create(:dossier, :en_construction, :with_dossier_link, procedure: procedure) }
+        before { subject }
+        context 'when the expert doesn’t share linked dossiers' do
+          let(:invite_linked_dossiers) { false }
 
-        context 'but the expert can’t access the linked dossier' do
           it 'sends a single avis for the main dossier, but doesn’t give access to the linked dossiers' do
             expect(flash.notice).to eq("Une demande d'avis a été envoyée à email@a.com")
             expect(Avis.count).to eq(old_avis_count + 1)
@@ -525,6 +510,40 @@ describe Instructeurs::DossiersController, type: :controller do
             expect(saved_avis.dossier).to eq(dossier)
           end
         end
+
+        context 'when the expert also shares the linked dossiers' do
+          let(:invite_linked_dossiers) { true }
+
+          context 'and the expert can access the linked dossiers' do
+            let(:saved_avis) { Avis.last(2).first }
+            let(:linked_avis) { Avis.last }
+            let(:linked_dossier) { Dossier.find_by(id: dossier.reload.champs.filter(&:dossier_link?).map(&:value).compact) }
+            let(:invite_linked_dossiers) do
+              instructeur.assign_to_procedure(linked_dossier.procedure)
+              true
+            end
+
+            it 'sends one avis for the main dossier' do
+              expect(flash.notice).to eq("Une demande d'avis a été envoyée à email@a.com")
+              expect(saved_avis.email).to eq("email@a.com")
+              expect(saved_avis.dossier).to eq(dossier)
+            end
+
+            it 'sends another avis for the linked dossiers' do
+              expect(Avis.count).to eq(old_avis_count + 2)
+              expect(linked_avis.dossier).to eq(linked_dossier)
+            end
+          end
+
+          context 'but the expert can’t access the linked dossier' do
+            it 'sends a single avis for the main dossier, but doesn’t give access to the linked dossiers' do
+              expect(flash.notice).to eq("Une demande d'avis a été envoyée à email@a.com")
+              expect(Avis.count).to eq(old_avis_count + 1)
+              expect(saved_avis.email).to eq("email@a.com")
+              expect(saved_avis.dossier).to eq(dossier)
+            end
+          end
+        end
       end
     end
   end
@@ -569,6 +588,7 @@ describe Instructeurs::DossiersController, type: :controller do
       ], instructeurs: instructeurs)
     end
     let(:dossier) { create(:dossier, :en_construction, :with_all_annotations, procedure: procedure) }
+    let(:another_instructeur) { create(:instructeur) }
     let(:now) { Time.zone.parse('01/01/2100') }
 
     let(:champ_multiple_drop_down_list) do
@@ -588,6 +608,7 @@ describe Instructeurs::DossiersController, type: :controller do
     end
 
     before do
+      another_instructeur.follow(dossier)
       Timecop.freeze(now)
       patch :update_annotations, params: params
 
@@ -646,6 +667,12 @@ describe Instructeurs::DossiersController, type: :controller do
         expect(dossier.reload.last_champ_private_updated_at).to eq(now)
         expect(response).to redirect_to(annotations_privees_instructeur_dossier_path(dossier.procedure, dossier))
       }
+
+      it 'updates the annotations' do
+        Timecop.travel(now + 1.hour)
+        expect(instructeur.followed_dossiers.with_notifications(instructeur)).to eq([])
+        expect(another_instructeur.followed_dossiers.with_notifications(instructeur)).to eq([dossier.reload])
+      end
     end
 
     context "without new values for champs_private" do

spec/controllers/users/dossiers_controller_spec.rb

@@ -682,6 +682,22 @@ describe Users::DossiersController, type: :controller do
       it { expect(dossier.reload.state).to eq(Dossier.states.fetch(:en_construction)) }
       it { expect(response).to redirect_to(demande_dossier_path(dossier)) }
     end
+
+    context 'when the dossier is followed by an instructeur' do
+      let(:dossier) { create(:dossier) }
+      let(:instructeur) { create(:instructeur) }
+      let!(:invite) { create(:invite, dossier: dossier, user: user) }
+
+      before do
+        instructeur.follow(dossier)
+      end
+
+      it 'the follower has a notification' do
+        expect(instructeur.reload.followed_dossiers.with_notifications(instructeur)).to eq([])
+        subject
+        expect(instructeur.reload.followed_dossiers.with_notifications(instructeur)).to eq([dossier.reload])
+      end
+    end
   end
 
   describe '#index' do
@@ -834,6 +850,8 @@ describe Users::DossiersController, type: :controller do
     before do
       Timecop.freeze(now)
       sign_in(user)
+      # Flipper.enable(:cached_notifications, instructeur_with_instant_message)
+      # Flipper.enable(:cached_notifications, instructeur_without_instant_message)
       allow(ClamavService).to receive(:safe_file?).and_return(scan_result)
       allow(DossierMailer).to receive(:notify_new_commentaire_to_instructeur).and_return(double(deliver_later: nil))
       instructeur_with_instant_message.follow(dossier)
@@ -844,14 +862,30 @@ describe Users::DossiersController, type: :controller do
 
     after { Timecop.return }
 
-    it "creates a commentaire" do
+    context 'commentaire creation' do
-      expect { subject }.to change(Commentaire, :count).by(1)
+      it "creates a commentaire" do
+        expect { subject }.to change(Commentaire, :count).by(1)
 
-      expect(response).to redirect_to(messagerie_dossier_path(dossier))
+        expect(response).to redirect_to(messagerie_dossier_path(dossier))
-      expect(DossierMailer).to have_received(:notify_new_commentaire_to_instructeur).with(dossier, instructeur_with_instant_message.email)
+        expect(DossierMailer).to have_received(:notify_new_commentaire_to_instructeur).with(dossier, instructeur_with_instant_message.email)
-      expect(DossierMailer).not_to have_received(:notify_new_commentaire_to_instructeur).with(dossier, instructeur_without_instant_message.email)
+        expect(DossierMailer).not_to have_received(:notify_new_commentaire_to_instructeur).with(dossier, instructeur_without_instant_message.email)
-      expect(flash.notice).to be_present
+        expect(flash.notice).to be_present
-      expect(dossier.reload.last_commentaire_updated_at).to eq(now)
+        expect(dossier.reload.last_commentaire_updated_at).to eq(now)
+      end
+    end
+
+    context 'notification' do
+      before 'instructeurs have no notification before the message' do
+        expect(instructeur_with_instant_message.followed_dossiers.with_notifications(instructeur_with_instant_message)).to eq([])
+        expect(instructeur_without_instant_message.followed_dossiers.with_notifications(instructeur_without_instant_message)).to eq([])
+        Timecop.travel(now + 1.day)
+        subject
+      end
+
+      it 'adds them a notification' do
+        expect(instructeur_with_instant_message.reload.followed_dossiers.with_notifications(instructeur_with_instant_message)).to eq([dossier.reload])
+        expect(instructeur_without_instant_message.reload.followed_dossiers.with_notifications(instructeur_without_instant_message)).to eq([dossier.reload])
+      end
     end
   end
 

spec/models/dossier_spec.rb

@@ -103,18 +103,39 @@ describe Dossier do
 
     before do
       create(:follow, dossier: dossier, instructeur: instructeur, messagerie_seen_at: 2.hours.ago)
+      Flipper.enable_actor(:cached_notifications, instructeur)
     end
 
-    subject { instructeur.followed_dossiers.with_notifications }
+    subject { instructeur.followed_dossiers.with_notifications(instructeur) }
 
     context('without changes') do
       it { is_expected.to eq [] }
     end
 
     context('with changes') do
-      before { dossier.commentaires << create(:commentaire, email: 'test@exemple.fr') }
+      context 'when there is a new commentaire' do
+        before { dossier.update!(last_commentaire_updated_at: Time.zone.now) }
 
-      it { is_expected.to match([dossier]) }
+        it { is_expected.to match([dossier]) }
+      end
+
+      context 'when there is a new avis' do
+        before { dossier.update!(last_avis_updated_at: Time.zone.now) }
+
+        it { is_expected.to match([dossier]) }
+      end
+
+      context 'when a public champ is updated' do
+        before { dossier.update!(last_champ_updated_at: Time.zone.now) }
+
+        it { is_expected.to match([dossier]) }
+      end
+
+      context 'when a private champ is updated' do
+        before { dossier.update!(last_champ_private_updated_at: Time.zone.now) }
+
+        it { is_expected.to match([dossier]) }
+      end
     end
   end
 

spec/models/instructeur_spec.rb

@@ -8,6 +8,7 @@ describe Instructeur, type: :model do
 
   before do
     assign(procedure_2)
+    Flipper.enable_actor(:cached_notifications, instructeur)
   end
 
   describe 'follow' do
@@ -263,12 +264,20 @@ describe Instructeur, type: :model do
 
     let!(:dossier_on_procedure_2) { create(:dossier, :followed, state: Dossier.states.fetch(:en_construction)) }
     let!(:instructeur_on_procedure_2) { dossier_on_procedure_2.follows.first.instructeur }
+    let(:now) { Time.zone.parse("14/09/1867") }
+    let(:follow) { instructeur.follows.find_by(dossier: dossier) }
+    let(:follow2) { instructeur_2.follows.find_by(dossier: dossier) }
+    let(:seen_at_instructeur) { now - 1.hour }
+    let(:seen_at_instructeur2) { now - 1.hour }
 
     before do
       procedure.groupe_instructeurs.last.instructeurs << instructeur
       instructeur_2.followed_dossiers << dossier
+      Timecop.freeze(now)
     end
 
+    after { Timecop.return }
+
     subject { instructeur.notifications_for_procedure(procedure, :en_cours) }
 
     context 'when the instructeur has just followed the dossier' do
@@ -276,7 +285,11 @@ describe Instructeur, type: :model do
     end
 
     context 'when there is a modification on public champs' do
-      before { dossier.champs.first.update_attribute('value', 'toto') }
+      before do
+        dossier.update!(last_champ_updated_at: now)
+        follow.update_attribute('demande_seen_at', seen_at_instructeur)
+        follow2.update_attribute('demande_seen_at', seen_at_instructeur2)
+      end
 
       it { is_expected.to match([dossier]) }
       it { expect(instructeur_2.notifications_for_procedure(procedure, :en_cours)).to match([dossier]) }
@@ -289,9 +302,8 @@ describe Instructeur, type: :model do
       end
 
       context 'when instructeur update it s public champs last seen' do
-        let(:follow) { instructeur.follows.find_by(dossier: dossier) }
+        let(:seen_at_instructeur) { now + 1.hour }
-
+        let(:seen_at_instructeur2) { now - 1.hour }
-        before { follow.update_attribute('demande_seen_at', Time.zone.now) }
 
         it { is_expected.to match([]) }
         it { expect(instructeur_2.notifications_for_procedure(procedure, :en_cours)).to match([dossier]) }
@@ -305,20 +317,29 @@ describe Instructeur, type: :model do
     end
 
     context 'when there is a modification on private champs' do
-      before { dossier.champs_private.first.update_attribute('value', 'toto') }
+      before do
+        dossier.update!(last_champ_private_updated_at: now)
+        follow.update_attribute('annotations_privees_seen_at', seen_at_instructeur)
+      end
 
       it { is_expected.to match([dossier]) }
     end
 
     context 'when there is a modification on avis' do
-      before { create(:avis, dossier: dossier) }
+      before do
+        dossier.update!(last_avis_updated_at: Time.zone.now)
+        follow.update_attribute('avis_seen_at', seen_at_instructeur)
+      end
 
       it { is_expected.to match([dossier]) }
     end
 
     context 'the messagerie' do
       context 'when there is a new commentaire' do
-        before { create(:commentaire, dossier: dossier, email: 'a@b.com') }
+        before do
+          dossier.update!(last_commentaire_updated_at: Time.zone.now)
+          follow.update_attribute('messagerie_seen_at', seen_at_instructeur)
+        end
 
         it { is_expected.to match([dossier]) }
       end
@@ -339,7 +360,7 @@ describe Instructeur, type: :model do
     subject { instructeur.procedures_with_notifications(:en_cours) }
 
     context 'when there is a modification on public champs' do
-      before { dossier.champs.first.update_attribute('value', 'toto') }
+      before { dossier.update!(last_champ_updated_at: Time.zone.now) }
 
       it { is_expected.to match([procedure]) }
     end

spec/models/procedure_presentation_spec.rb

@@ -262,7 +262,8 @@ describe ProcedurePresentation do
       let!(:older_dossier) { create(:dossier, :en_construction, procedure: procedure) }
 
       before do
-        notified_dossier.champs.first.touch(time: Time.zone.local(2018, 9, 20))
+        Flipper.enable_actor(:cached_notifications, instructeur)
+        notified_dossier.update!(last_champ_updated_at: Time.zone.local(2018, 9, 20))
         create(:follow, instructeur: instructeur, dossier: notified_dossier, demande_seen_at: Time.zone.local(2018, 9, 10))
         notified_dossier.touch(time: Time.zone.local(2018, 9, 20))
         recent_dossier.touch(time: Time.zone.local(2018, 9, 25))