Merge branch 'revert-attachment-masking-uniqueness' into develop

Gemfile

@@ -87,6 +87,7 @@ gem 'pg', '~> 1.5.4'
 gem 'acts_as_versioned', git: 'https://github.com/mysociety/acts_as_versioned.git',
                          ref: '13e928b'
 gem 'active_model_otp'
+gem 'activejob-uniqueness', '~> 0.2.5'
 gem 'bcrypt', '~> 3.1.20'
 gem 'cancancan', '~> 3.5.0'
 gem 'charlock_holmes', '~> 0.7.7'

Gemfile.lock

@@ -113,6 +113,9 @@ GEM
     activejob (7.0.8)
       activesupport (= 7.0.8)
       globalid (>= 0.3.6)
+    activejob-uniqueness (0.2.5)
+      activejob (>= 4.2, < 7.1)
+      redlock (>= 1.2, < 2)
     activemodel (7.0.8)
       activesupport (= 7.0.8)
     activerecord (7.0.8)
@@ -187,7 +190,7 @@ GEM
       xpath (~> 3.2)
     charlock_holmes (0.7.7)
     coderay (1.1.3)
-    concurrent-ruby (1.2.2)
+    concurrent-ruby (1.2.3)
     connection_pool (2.4.1)
     crack (0.4.5)
       rexml
@@ -327,7 +330,7 @@ GEM
     mini_portile2 (2.8.5)
     mini_racer (0.8.0)
       libv8-node (~> 18.16.0.0)
-    minitest (5.20.0)
+    minitest (5.21.2)
     money (6.16.0)
       i18n (>= 0.6.4, <= 2)
     multi_json (1.15.0)
@@ -420,6 +423,8 @@ GEM
     recaptcha (5.16.0)
     redcarpet (3.6.0)
     redis (4.8.1)
+    redlock (1.3.2)
+      redis (>= 3.0.0, < 6.0)
     regexp_parser (2.8.3)
     representable (3.2.0)
       declarative (< 0.1.0)
@@ -568,6 +573,7 @@ PLATFORMS
 
 DEPENDENCIES
   active_model_otp
+  activejob-uniqueness (~> 0.2.5)
   acts_as_versioned!
   alaveteli_features!
   annotate (< 3.2.1)

app/controllers/attachment_masks_controller.rb

@@ -17,7 +17,7 @@ def wait
       )
 
     else
-      FoiAttachmentMaskJob.perform_once_later(@attachment)
+      FoiAttachmentMaskJob.perform_later(@attachment)
     end
   end
 

app/controllers/attachments_controller.rb

@@ -123,7 +123,7 @@ def ensure_masked
     if @attachment.masked?
       yield
     else
-      FoiAttachmentMaskJob.perform_once_later(@attachment)
+      FoiAttachmentMaskJob.perform_later(@attachment)
 
       Timeout.timeout(5) do
         until @attachment.masked?

app/jobs/foi_attachment_mask_job.rb

@@ -6,9 +6,8 @@
 #   FoiAttachmentMaskJob.perform(FoiAttachment.first)
 #
 class FoiAttachmentMaskJob < ApplicationJob
-  include Uniqueness
-
   queue_as :default
+  unique :until_and_while_executing, on_conflict: :log
 
   attr_reader :attachment
 

app/models/foi_attachment.rb

@@ -115,7 +115,8 @@ def body
     end
 
     if persisted?
-      FoiAttachmentMaskJob.perform_once_now(self)
+      FoiAttachmentMaskJob.unlock!(self)
+      FoiAttachmentMaskJob.perform_now(self)
       return body unless destroyed?
     end
 

config/initializers/active_job_uniqueness.rb

@@ -0,0 +1,5 @@
+require 'redis_connection'
+
+ActiveJob::Uniqueness.configure do |config|
+  config.redlock_servers = [RedisConnection.instance]
+end

doc/CHANGES.md

@@ -2,6 +2,7 @@
 
 ## Highlighted Features
 
+* Fix duplicated attachment masking jobs (Graeme Porteous)
 * Display metadata on admin attachment views (Graeme Porteous)
 * Change request URL patterns to be member routes (Alexander Griffen, Graeme
   Porteous)

lib/redis_connection.rb

@@ -1,7 +1,8 @@
 require File.expand_path('../config/load_env.rb', __dir__)
 
 ##
-# Module to parse Redis ENV variables into usable configuration for Sidekiq.
+# Module to parse Redis ENV variables into usable configuration for Sidekiq and
+# ActiveJob::Uniqueness gems.
 #
 module RedisConnection
   def self.instance

spec/jobs/foi_attachment_mask_job_spec.rb

@@ -139,57 +139,4 @@ def perform
       expect(new_attachment.body).to include 'Banana'
     end
   end
-
-  describe '.perform_once_later' do
-    it 'call perform_later' do
-      expect(FoiAttachmentMaskJob).to receive(:perform_later)
-      FoiAttachmentMaskJob.perform_once_later(attachment)
-    end
-
-    it 'does not call perform_later if existing job is present' do
-      allow(FoiAttachmentMaskJob).to receive(:existing_job).and_return(double)
-      expect(FoiAttachmentMaskJob).to_not receive(:perform_later)
-      FoiAttachmentMaskJob.perform_once_later(attachment)
-    end
-  end
-
-  describe '.perform_once_now' do
-    it 'deleted existing job if present' do
-      job = double(:job)
-      allow(FoiAttachmentMaskJob).to receive(:existing_job).and_return(job)
-      expect(job).to receive(:delete)
-      FoiAttachmentMaskJob.perform_once_now(attachment)
-    end
-
-    it 'calls perform_now' do
-      expect(FoiAttachmentMaskJob).to receive(:perform_now)
-      FoiAttachmentMaskJob.perform_once_now(attachment)
-    end
-  end
-
-  describe '.existing_job' do
-    around do |example|
-      adapter = ActiveJob::Base.queue_adapter
-      ActiveJob::Base.queue_adapter = :sidekiq
-      example.call
-      ActiveJob::Base.queue_adapter = adapter
-    end
-
-    before do
-      allow(FoiAttachmentMaskJob).to receive(:queue_name).and_return('test')
-    end
-
-    after do
-      Sidekiq::Queue.new('test').clear
-    end
-
-    it 'return nil if existing job is not present' do
-      expect(FoiAttachmentMaskJob.existing_job(attachment)).to be_nil
-    end
-
-    it 'return existing job if present' do
-      FoiAttachmentMaskJob.perform_later(attachment)
-      expect(FoiAttachmentMaskJob.existing_job(attachment)).to_not be_nil
-    end
-  end
 end