Merge pull request #3818 from tchak/migrate-message-attachement-to-active-storage

Migrate message attachements to use active_storage

app/controllers/gestionnaires/avis_controller.rb

@@ -138,7 +138,7 @@ module Gestionnaires
     end
 
     def commentaire_params
-      params.require(:commentaire).permit(:body, :file)
+      params.require(:commentaire).permit(:body, :piece_jointe)
     end
   end
 end

app/controllers/gestionnaires/dossiers_controller.rb

@@ -166,7 +166,7 @@ module Gestionnaires
     end
 
     def commentaire_params
-      params.require(:commentaire).permit(:body, :file)
+      params.require(:commentaire).permit(:body, :piece_jointe)
     end
 
     def champs_private_params

app/controllers/support_controller.rb

@@ -52,7 +52,7 @@ class SupportController < ApplicationController
       email: email,
       phone: params[:phone],
       text: params[:text],
-      file: params[:file],
+      file: params[:piece_jointe],
       dossier_id: dossier&.id,
       browser: browser_name,
       tags: tags
@@ -61,7 +61,7 @@ class SupportController < ApplicationController
 
   def create_commentaire
     attributes = {
-      file: params[:file],
+      piece_jointe: params[:piece_jointe],
       body: "[#{params[:subject]}]<br><br>#{params[:text]}"
     }
     commentaire = CommentaireService.build(current_user, dossier, attributes)

app/controllers/users/dossiers_controller.rb

@@ -354,7 +354,7 @@ module Users
     end
 
     def commentaire_params
-      params.require(:commentaire).permit(:body, :file)
+      params.require(:commentaire).permit(:body, :piece_jointe)
     end
 
     def passage_en_construction?

app/models/commentaire.rb

@@ -5,9 +5,10 @@ class Commentaire < ApplicationRecord
   belongs_to :gestionnaire
 
   mount_uploader :file, CommentaireFileUploader
-  validates :file, file_size: { maximum: 20.megabytes, message: "La taille du fichier doit être inférieure à 20 Mo" }
-  validate :is_virus_free?
   validate :messagerie_available?, on: :create
+
+  has_one_attached :piece_jointe
+
   validates :body, presence: { message: "Votre message ne peut être vide" }
 
   default_scope { order(created_at: :asc) }
@@ -47,10 +48,15 @@ class Commentaire < ApplicationRecord
   end
 
   def file_url
-    if Flipflop.remote_storage?
+    if piece_jointe.attached?
+      if piece_jointe.virus_scanner.safe?
+        Rails.application.routes.url_helpers.url_for(piece_jointe)
+      end
+    elsif Flipflop.remote_storage?
       RemoteDownloader.new(file.path).url
-    else
-      file.url
+    elsif file&.url
+      # FIXME: this is horrible but used only in dev and will be removed after migration
+      File.join(LOCAL_DOWNLOAD_URL, file.url)
     end
   end
 
@@ -74,12 +80,6 @@ class Commentaire < ApplicationRecord
     DossierMailer.notify_new_answer(dossier).deliver_later
   end
 
-  def is_virus_free?
-    if file.present? && file_changed? && !ClamavService.safe_file?(file.path)
-      errors.add(:file, "Virus détecté dans le fichier joint, merci de changer de fichier")
-    end
-  end
-
   def messagerie_available?
     return if sent_by_system?
     if dossier.present? && !dossier.messagerie_available?

app/models/dossier.rb

@@ -117,7 +117,7 @@ class Dossier < ApplicationRecord
   scope :nearing_end_of_retention,    -> (duration = '1 month') { joins(:procedure).where("en_instruction_at + (duree_conservation_dossiers_dans_ds * interval '1 month') - now() < interval ?", duration) }
   scope :since,                       -> (since) { where('dossiers.en_construction_at >= ?', since) }
   scope :for_api, -> {
-    includes(commentaires: [],
+    includes(commentaires: { piece_jointe_attachment: :blob },
       champs: [
         :geo_areas,
         :etablissement,

app/services/commentaire_service.rb

@@ -13,6 +13,11 @@ class CommentaireService
 
     def build_with_email(email, dossier, params)
       attributes = params.merge(email: email, dossier: dossier)
+      # For some reason ActiveStorage trows an error in tests if we passe an empty string here.
+      # I suspect it could be resolved in rails 6 by using explicit `attach()`
+      if attributes[:piece_jointe].blank?
+        attributes.delete(:piece_jointe)
+      end
       Commentaire.new(attributes)
     end
   end

app/uploaders/local_downloader.rb

@@ -14,7 +14,7 @@ class LocalDownloader
   end
 
   def url
-    @url ||= File.join(TPS::Application::URL, 'downloads', random_folder_name, "#{@filename_suffix}.#{@extension}")
+    @url ||= File.join(LOCAL_DOWNLOAD_URL, 'downloads', random_folder_name, "#{@filename_suffix}.#{@extension}")
   end
 
   protected

app/views/gestionnaires/dossiers/print.html.haml

@@ -61,7 +61,7 @@
 
 .messagerie
   %ul.messages-list
-    - @dossier.commentaires.each do |commentaire|
+    - @dossier.commentaires.with_attached_piece_jointe.each do |commentaire|
       %li
         = render partial: "shared/dossiers/messages/message", locals: { commentaire: commentaire, connected_user: current_gestionnaire, messagerie_seen_at: nil }
 

app/views/shared/dossiers/_messagerie.html.haml

@@ -1,6 +1,6 @@
 .messagerie.container
   %ul.messages-list
-    - dossier.commentaires.each do |commentaire|
+    - dossier.commentaires.with_attached_piece_jointe.each do |commentaire|
       %li.message{ class: commentaire_is_from_me_class(commentaire, connected_user) }
         = render partial: "shared/dossiers/messages/message", locals: { commentaire: commentaire, connected_user: connected_user, messagerie_seen_at: messagerie_seen_at }
 

app/views/shared/dossiers/messages/_form.html.haml

@@ -2,8 +2,8 @@
   = f.text_area :body, rows: 5, placeholder: 'Répondre ici', required: true, class: 'message-textarea'
   .flex.justify-between.wrap
     %div
-      = f.file_field :file, id: :file, accept: commentaire.file.accept_extension_list
-      %label{ for: :file }
+      = f.file_field :piece_jointe, id: 'piece_jointe', direct_upload: true
+      %label{ for: :piece_jointe }
         .notice
           (taille max : 20 Mo)
 

app/views/shared/dossiers/messages/_message.html.haml

@@ -10,7 +10,10 @@
       = commentaire_date(commentaire)
   .rich-text= sanitize(simple_format(commentaire.body))
 
-  - if commentaire.file.present?
+  - if commentaire.piece_jointe.attached?
+    .attachment-link
+      = render partial: "shared/attachment/show", locals: { attachment: commentaire.piece_jointe.attachment }
+  - elsif commentaire.file.present?
     .attachment-link
       = link_to commentaire.file_url, class: "button", target: "_blank", rel: "noopener", title: "Télécharger" do
         %span.icon.attachment

app/views/support/index.html.haml

@@ -76,7 +76,7 @@
             Une capture d’écran peut nous aider à identifier plus facilement l’endroit à améliorer.
           .notice.hidden{ data: { 'contact-type-only': Helpscout::FormAdapter::TYPE_AUTRE } }
             Une capture d’écran peut nous aider à identifier plus facilement le problème.
-        = file_field_tag :file
+        = file_field_tag :piece_jointe
 
       = hidden_field_tag :tags, @tags&.join(',')
 

config/application.rb

@@ -31,8 +31,6 @@ module TPS
     config.assets.paths << Rails.root.join('app', 'assets', 'fonts')
     config.assets.precompile += ['.woff']
 
-    URL = ENV['APP_HOST'] || "http://localhost:3000/"
-
     config.active_job.queue_adapter = :delayed_job
 
     config.action_view.sanitized_allowed_tags = ActionView::Base.sanitized_allowed_tags + ['u']

config/initializers/urls.rb

@@ -28,3 +28,6 @@ FAQ_ADMIN_URL = "https://faq.demarches-simplifiees.fr/collection/1-administrateu
 COMMENT_TROUVER_MA_DEMARCHE_URL = [FAQ_URL, "article", "59-comment-trouver-ma-demarche"].join("/")
 STATUS_PAGE_URL = "https://status.demarches-simplifiees.fr"
 MATOMO_IFRAME_URL = "https://stats.data.gouv.fr/index.php?module=CoreAdminHome&action=optOut&language=fr&&fontColor=333333&fontSize=16px&fontFamily=Muli"
+
+# FIXME: This is only used in dev in couple of places and should be removed after PJ migration
+LOCAL_DOWNLOAD_URL = "http://#{ENV.fetch('APP_HOST', 'localhost:3000')}"

lib/tasks/2019_05_29_migrate_commentaire_pj.rake

@@ -0,0 +1,37 @@
+namespace :'2019_05_29_migrate_commentaire_pj' do
+  task run: :environment do
+    commentaires = Commentaire.where
+      .not(file: nil)
+      .left_joins(:piece_jointe_attachment)
+      .where('active_storage_attachments.id IS NULL')
+      .order(:created_at)
+
+    limit = ENV['LIMIT']
+    if limit
+      commentaires.limit!(limit.to_i)
+    end
+
+    progress = ProgressReport.new(commentaires.count)
+    commentaires.find_each do |commentaire|
+      if commentaire.file.present?
+        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
+          commentaire.piece_jointe.attach(
+            io: StringIO.new(response.body),
+            filename: filename,
+            content_type: commentaire.file.content_type,
+            metadata: { virus_scan_result: ActiveStorage::VirusScanner::SAFE }
+          )
+          commentaire.update_column(:updated_at, updated_at)
+          commentaire.dossier.update_column(:updated_at, dossier_updated_at)
+        end
+      end
+      progress.inc
+    end
+    progress.finish
+  end
+end

spec/controllers/gestionnaires/avis_controller_spec.rb

@@ -92,7 +92,7 @@ describe Gestionnaires::AvisController, type: :controller do
       let(:file) { nil }
       let(:scan_result) { true }
 
-      subject { post :create_commentaire, params: { id: avis_without_answer.id, commentaire: { body: 'commentaire body', file: file } } }
+      subject { post :create_commentaire, params: { id: avis_without_answer.id, commentaire: { body: 'commentaire body', piece_jointe: file } } }
 
       before do
         allow(ClamavService).to receive(:safe_file?).and_return(scan_result)
@@ -110,16 +110,10 @@ describe Gestionnaires::AvisController, type: :controller do
 
         it do
           subject
-          expect(Commentaire.last.file.path).to include("piece_justificative_0.pdf")
+          expect(Commentaire.last.piece_jointe.filename).to eq("piece_justificative_0.pdf")
         end
 
         it { expect { subject }.to change(Commentaire, :count).by(1) }
-
-        context "and a virus" do
-          let(:scan_result) { false }
-
-          it { expect { subject }.not_to change(Commentaire, :count) }
-        end
       end
     end
 

spec/controllers/gestionnaires/dossiers_controller_spec.rb

@@ -350,15 +350,15 @@ describe Gestionnaires::DossiersController, type: :controller do
       expect(flash.notice).to be_present
     end
 
-    context "when the commentaire creation fails" do
+    context "when the commentaire created with virus file" do
       let(:scan_result) { false }
 
-      it "renders the messagerie page with the invalid commentaire" do
-        expect { subject }.not_to change(Commentaire, :count)
+      it "creates a commentaire (shows message that file have a virus)" do
+        expect { subject }.to change(Commentaire, :count).by(1)
+        expect(gestionnaire.followed_dossiers).to include(dossier)
 
-        expect(response).to render_template :messagerie
-        expect(flash.alert).to be_present
-        expect(assigns(:commentaire).body).to eq("avant\napres")
+        expect(response).to redirect_to(messagerie_gestionnaire_dossier_path(dossier.procedure, dossier))
+        expect(flash.notice).to be_present
       end
     end
   end

spec/controllers/users/dossiers_controller_spec.rb

@@ -806,7 +806,7 @@ describe Users::DossiersController, type: :controller do
         id: dossier.id,
         commentaire: {
           body: body,
-          file: file
+          piece_jointe: file
         }
       }
     }
@@ -822,18 +822,6 @@ describe Users::DossiersController, type: :controller do
       expect(response).to redirect_to(messagerie_dossier_path(dossier))
       expect(flash.notice).to be_present
     end
-
-    context "when the commentaire creation fails" do
-      let(:scan_result) { false }
-
-      it "renders the messagerie page with the invalid commentaire" do
-        expect { subject }.not_to change(Commentaire, :count)
-
-        expect(response).to render_template :messagerie
-        expect(flash.alert).to be_present
-        expect(assigns(:commentaire).body).to eq("avant\napres")
-      end
-    end
   end
 
   describe '#ask_deletion' do

spec/factories/commentaire.rb

@@ -7,5 +7,9 @@ FactoryBot.define do
         commentaire.dossier = create :dossier, :en_construction
       end
     end
+
+    trait :with_file do
+      file { Rack::Test::UploadedFile.new("./spec/fixtures/files/logo_test_procedure.png", 'application/pdf') }
+    end
   end
 end

spec/lib/tasks/2019_05_29_migrate_commentaire_pj_spec.rb

@@ -0,0 +1,42 @@
+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)
+  end
+
+  before do
+    Commentaire.all.each do |commentaire|
+      if commentaire.file.present?
+        stub_request(:get, commentaire.file_url)
+          .to_return(status: 200, body: File.read(commentaire.file.path))
+      end
+    end
+  end
+
+  after do
+    ENV['LIMIT'] = nil
+    rake_task.reenable
+  end
+
+  it 'should migrate pj' do
+    comment_updated_at = Commentaire.last.updated_at
+    dossier_updated_at = Commentaire.last.dossier.updated_at
+    expect(Commentaire.all.map(&:piece_jointe).map(&:attached?)).to eq([false, false, false])
+    rake_task.invoke
+    expect(Commentaire.where(file: nil).count).to eq(1)
+    expect(Commentaire.all.map(&:piece_jointe).map(&:attached?)).to eq([false, true, true])
+    expect(Commentaire.last.updated_at).to eq(comment_updated_at)
+    expect(Commentaire.last.dossier.updated_at).to eq(dossier_updated_at)
+  end
+
+  it 'should migrate pj within limit' do
+    expect(Commentaire.all.map(&:piece_jointe).map(&:attached?)).to eq([false, false, false])
+    ENV['LIMIT'] = '1'
+    rake_task.invoke
+    expect(Commentaire.where(file: nil).count).to eq(1)
+    expect(Commentaire.all.map(&:piece_jointe).map(&:attached?)).to eq([false, true, false])
+  end
+end

spec/services/commentaire_service_spec.rb

@@ -1,24 +1,21 @@
 require 'spec_helper'
 
 describe CommentaireService do
+  include ActiveJob::TestHelper
+
   describe '.create' do
     let(:dossier) { create :dossier, :en_construction }
     let(:sender) { dossier.user }
     let(:body) { 'Contenu du message.' }
     let(:file) { nil }
-    let(:scan_result) { true }
 
-    subject(:commentaire) { CommentaireService.build(sender, dossier, { body: body, file: file }) }
-
-    before do
-      allow(ClamavService).to receive(:safe_file?).and_return(scan_result)
-    end
+    subject(:commentaire) { CommentaireService.build(sender, dossier, { body: body, piece_jointe: file }) }
 
     it 'creates a new valid commentaire' do
       expect(commentaire.email).to eq sender.email
       expect(commentaire.dossier).to eq dossier
       expect(commentaire.body).to eq 'Contenu du message.'
-      expect(commentaire.file).to be_blank
+      expect(commentaire.piece_jointe.attached?).to be_falsey
       expect(commentaire).to be_valid
     end
 
@@ -34,14 +31,15 @@ describe CommentaireService do
     context 'when it has a file' do
       let(:file) { Rack::Test::UploadedFile.new("./spec/fixtures/files/piece_justificative_0.pdf", 'application/pdf') }
 
-      it 'saves the attached file' do
-        expect(commentaire.file).to be_present
-        expect(commentaire).to be_valid
+      before do
+        expect(ClamavService).to receive(:safe_file?).and_return(true)
       end
 
-      context 'and a virus' do
-        let(:scan_result) { false }
-        it { expect(commentaire).not_to be_valid }
+      it 'saves the attached file' do
+        perform_enqueued_jobs do
+          commentaire.save
+          expect(commentaire.piece_jointe.attached?).to be_truthy
+        end
       end
     end
   end