active_storage: fix blob update hooks

For some reason on Rails 6.1 the `after_update_commit` hook is properly
registered – but disappears from the record later, and in the end is
never run.

Fix it by using the general `after_commit` hook instead.

app/models/concerns/blob_titre_identite_watermark_concern.rb

@@ -1,8 +1,19 @@
+# Request a watermark on blobs attached to a `Champs::TitreIdentiteChamp`
+# after the virus scan has run.
+#
+# We're using a class extension here, but we could as well have a periodic
+# job that watermarks relevant attachments.
+#
+# The `after_commit` hook is triggered, among other cases, when
+# the analyzer or virus scan updates the blob metadata. When both the analyzer
+# and the virus scan have run, it is now safe to start the watermarking,
+# without  risking to replace the picture while it is being scanned in a
+# concurrent job.
 module BlobTitreIdentiteWatermarkConcern
   extend ActiveSupport::Concern
 
   included do
-    after_update_commit :enqueue_watermark_job
+    after_commit :enqueue_watermark_job
   end
 
   def watermark_pending?
@@ -12,7 +23,7 @@ module BlobTitreIdentiteWatermarkConcern
   private
 
   def watermark_required?
-    attachments.find { |attachment| attachment.record.class.name == 'Champs::TitreIdentiteChamp' }
+    attachments.any? { |attachment| attachment.record.class.name == 'Champs::TitreIdentiteChamp' }
   end
 
   def watermark_done?

app/models/concerns/blob_virus_scanner_concern.rb

@@ -1,13 +1,21 @@
-# TODO: once we're using Rails 6, use the hooks on attachments creation
-# (rather than on blob creation).
-# This will help to avoid cloberring metadata accidentally (as metadata
-# are more stable on attachment creation than on blob creation).
+# Run a virus scan on all blobs after they are analyzed.
+#
+# We're using a class extension to ensure that all blobs get scanned,
+# regardless on how they were created. This could be an ActiveStorage::Analyzer,
+# but as of Rails 6.1 only the first matching analyzer is ever run on
+# a blob (and we may want to analyze the dimension of a picture as well
+# as scanning it).
+#
+# The `after_commit` hook is triggered, among other cases, when
+# the analyzer updates the blob metadata. When the analyzer has run,
+# it is now safe to start our own scanning, without risking to have
+# two concurrent jobs overwriting the metadata of the blob.
 module BlobVirusScannerConcern
   extend ActiveSupport::Concern
 
   included do
     before_create :set_pending
-    after_update_commit :enqueue_virus_scan
+    after_commit :enqueue_virus_scan
   end
 
   def virus_scanner

spec/models/champ_spec.rb

@@ -1,4 +1,6 @@
 describe Champ do
+  include ActiveJob::TestHelper
+
   require 'models/champ_shared_example.rb'
 
   it_should_behave_like "champ_spec"
@@ -435,19 +437,56 @@ describe Champ do
     end
   end
 
-  describe '#enqueue_virus_check' do
-    let(:champ) { build(:champ_piece_justificative, type_de_champ: type_de_champ) }
-
+  describe '#enqueue_virus_scan' do
     context 'when type_champ is type_de_champ_piece_justificative' do
       let(:type_de_champ) { create(:type_de_champ_piece_justificative) }
+      let(:champ) { build(:champ_piece_justificative, type_de_champ: type_de_champ) }
 
       context 'and there is a blob' do
         before do
-          champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain")
-          champ.save!
+          allow(ClamavService).to receive(:safe_file?).and_return(true)
         end
 
-        it { expect(champ.piece_justificative_file.virus_scanner.started?).to be_truthy }
+        subject do
+          champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain")
+          champ.save!
+          champ
+        end
+
+        it 'marks the file as pending virus scan' do
+          expect(subject.piece_justificative_file.virus_scanner.started?).to be_truthy
+        end
+
+        it 'marks the file as safe once the scan completes' do
+          perform_enqueued_jobs { subject }
+          expect(champ.reload.piece_justificative_file.virus_scanner.safe?).to be_truthy
+        end
+      end
+    end
+  end
+
+  describe '#enqueue_watermark_job' do
+    context 'when type_champ is type_de_champ_titre_identite' do
+      let(:type_de_champ) { create(:type_de_champ_titre_identite) }
+      let(:champ) { build(:champ_titre_identite, type_de_champ: type_de_champ) }
+
+      before do
+        allow(ClamavService).to receive(:safe_file?).and_return(true)
+      end
+
+      subject do
+        champ.piece_justificative_file.attach(fixture_file_upload('spec/fixtures/files/logo_test_procedure.png', 'image/png'))
+        champ.save!
+        champ
+      end
+
+      it 'enqueues a watermark job on file attachment' do
+        expect(subject.piece_justificative_file.watermark_pending?).to be_truthy
+      end
+
+      it 'watermarks the file' do
+        perform_enqueued_jobs { subject }
+        expect(champ.reload.piece_justificative_file.blob.metadata[:watermark]).to be_truthy
       end
     end
   end
@@ -535,7 +574,6 @@ describe Champ do
     end
 
     context "fetch_external_data_later" do
-      include ActiveJob::TestHelper
       let(:data) { 'some other data' }
 
       it "fill data from external source" do