Use Geo::AttachmentRegistryFinder on Geo node status page

app/models/geo_node.rb

@@ -205,21 +205,6 @@ class GeoNode < ActiveRecord::Base
     Geo::FileRegistry.lfs_objects.failed.count
   end
 
-  def attachments_synced_count
-    return unless secondary?
-
-    upload_ids = uploads.pluck(:id)
-    synced_ids = Geo::FileRegistry.attachments.synced.pluck(:file_id)
-
-    (synced_ids & upload_ids).length
-  end
-
-  def attachments_failed_count
-    return unless secondary?
-
-    Geo::FileRegistry.attachments.failed.count
-  end
-
   def find_or_build_status
     status || build_status
   end

app/models/geo_node_status.rb

@@ -27,7 +27,6 @@ class GeoNodeStatus < ActiveRecord::Base
 
   def self.current_node_status
     current_node = Gitlab::Geo.current_node
-
     return unless current_node
 
     status = current_node.find_or_build_status
@@ -77,8 +76,10 @@ class GeoNodeStatus < ActiveRecord::Base
       self.repositories_failed_count = geo_node.project_registries.failed.count
       self.lfs_objects_synced_count = geo_node.lfs_objects_synced_count
       self.lfs_objects_failed_count = geo_node.lfs_objects_failed_count
-      self.attachments_synced_count = geo_node.attachments_synced_count
-      self.attachments_failed_count = geo_node.attachments_failed_count
+
+      attachments_finder = Geo::AttachmentRegistryFinder.new(current_node: geo_node)
+      self.attachments_synced_count = attachments_finder.find_synced_attachments.count
+      self.attachments_failed_count = attachments_finder.find_failed_attachments.count
     end
 
     self

spec/ee/spec/finders/geo/attachment_registry_finder_spec.rb

@@ -17,7 +17,8 @@ describe Geo::AttachmentRegistryFinder, :geo, :truncate do
   let(:upload_3) { create(:upload, :issuable_upload, model: synced_project) }
   let(:upload_4) { create(:upload, model: unsynced_project) }
   let(:upload_5) { create(:upload, model: synced_project) }
-  let(:upload_6) { create(:upload, :personal_snippet, model: synced_project) }
+  let(:upload_6) { create(:upload, :personal_snippet) }
+  let(:lfs_object) { create(:lfs_object) }
 
   subject { described_class.new(current_node: secondary) }
 
@@ -37,11 +38,12 @@ describe Geo::AttachmentRegistryFinder, :geo, :truncate do
         subject.find_synced_attachments
       end
 
-      it 'returns uploaded files that sync succeed' do
+      it 'returns synced avatars, attachment, personal snippets and files' do
         create(:geo_file_registry, :avatar, file_id: upload_1.id)
         create(:geo_file_registry, :avatar, file_id: upload_2.id)
         create(:geo_file_registry, :avatar, file_id: upload_3.id, success: false)
         create(:geo_file_registry, :avatar, file_id: upload_6.id)
+        create(:geo_file_registry, :lfs, file_id: lfs_object.id)
 
         synced_attachments = subject.find_synced_attachments
 
@@ -49,13 +51,14 @@ describe Geo::AttachmentRegistryFinder, :geo, :truncate do
       end
 
       context 'with selective sync' do
-        it 'returns uploaded files that belong to the namespaces' do
+        it 'returns synced avatars, attachment, personal snippets and files' do
           create(:geo_file_registry, :avatar, file_id: upload_1.id)
           create(:geo_file_registry, :avatar, file_id: upload_2.id)
           create(:geo_file_registry, :avatar, file_id: upload_3.id)
           create(:geo_file_registry, :avatar, file_id: upload_4.id)
           create(:geo_file_registry, :avatar, file_id: upload_5.id, success: false)
           create(:geo_file_registry, :avatar, file_id: upload_6.id)
+          create(:geo_file_registry, :lfs, file_id: lfs_object.id)
 
           secondary.update_attribute(:namespaces, [synced_group])
 
@@ -73,11 +76,12 @@ describe Geo::AttachmentRegistryFinder, :geo, :truncate do
         subject.find_failed_attachments
       end
 
-      it 'returns uploaded files that sync failed' do
+      it 'returns failed avatars, attachment, personal snippets and files' do
         create(:geo_file_registry, :avatar, file_id: upload_1.id)
         create(:geo_file_registry, :avatar, file_id: upload_2.id)
         create(:geo_file_registry, :avatar, file_id: upload_3.id, success: false)
         create(:geo_file_registry, :avatar, file_id: upload_6.id, success: false)
+        create(:geo_file_registry, :lfs, file_id: lfs_object.id, success: false)
 
         failed_attachments = subject.find_failed_attachments
 
@@ -85,13 +89,14 @@ describe Geo::AttachmentRegistryFinder, :geo, :truncate do
       end
 
       context 'with selective sync' do
-        it 'returns uploaded files that belong to the namespaces' do
+        it 'returns failed avatars, attachment, personal snippets and files' do
           create(:geo_file_registry, :avatar, file_id: upload_1.id, success: false)
           create(:geo_file_registry, :avatar, file_id: upload_2.id)
           create(:geo_file_registry, :avatar, file_id: upload_3.id, success: false)
           create(:geo_file_registry, :avatar, file_id: upload_4.id)
           create(:geo_file_registry, :avatar, file_id: upload_5.id)
           create(:geo_file_registry, :avatar, file_id: upload_6.id, success: false)
+          create(:geo_file_registry, :lfs, file_id: lfs_object.id, success: false)
 
           secondary.update_attribute(:namespaces, [synced_group])
 
@@ -115,11 +120,12 @@ describe Geo::AttachmentRegistryFinder, :geo, :truncate do
         subject.find_synced_attachments
       end
 
-      it 'returns uploaded files that sync succeed' do
+      it 'returns synced avatars, attachment, personal snippets and files' do
         create(:geo_file_registry, :avatar, file_id: upload_1.id)
         create(:geo_file_registry, :avatar, file_id: upload_2.id)
         create(:geo_file_registry, :avatar, file_id: upload_3.id, success: false)
         create(:geo_file_registry, :avatar, file_id: upload_6.id)
+        create(:geo_file_registry, :lfs, file_id: lfs_object.id)
 
         synced_attachments = subject.find_synced_attachments
 
@@ -127,13 +133,14 @@ describe Geo::AttachmentRegistryFinder, :geo, :truncate do
       end
 
       context 'with selective sync' do
-        it 'returns uploaded files that belong to the namespaces' do
+        it 'returns synced avatars, attachment, personal snippets and files' do
           create(:geo_file_registry, :avatar, file_id: upload_1.id)
           create(:geo_file_registry, :avatar, file_id: upload_2.id)
           create(:geo_file_registry, :avatar, file_id: upload_3.id)
           create(:geo_file_registry, :avatar, file_id: upload_4.id)
           create(:geo_file_registry, :avatar, file_id: upload_5.id, success: false)
           create(:geo_file_registry, :avatar, file_id: upload_6.id)
+          create(:geo_file_registry, :lfs, file_id: lfs_object.id)
 
           secondary.update_attribute(:namespaces, [synced_group])
 
@@ -151,11 +158,12 @@ describe Geo::AttachmentRegistryFinder, :geo, :truncate do
         subject.find_failed_attachments
       end
 
-      it 'returns uploaded files that sync succeed' do
+      it 'returns failed avatars, attachment, personal snippets and files' do
         create(:geo_file_registry, :avatar, file_id: upload_1.id)
         create(:geo_file_registry, :avatar, file_id: upload_2.id)
         create(:geo_file_registry, :avatar, file_id: upload_3.id, success: false)
         create(:geo_file_registry, :avatar, file_id: upload_6.id, success: false)
+        create(:geo_file_registry, :lfs, file_id: lfs_object.id, success: false)
 
         failed_attachments = subject.find_failed_attachments
 
@@ -163,13 +171,14 @@ describe Geo::AttachmentRegistryFinder, :geo, :truncate do
       end
 
       context 'with selective sync' do
-        it 'returns uploaded files that belong to the namespaces' do
+        it 'returns failed avatars, attachment, personal snippets and files' do
           create(:geo_file_registry, :avatar, file_id: upload_1.id, success: false)
           create(:geo_file_registry, :avatar, file_id: upload_2.id)
           create(:geo_file_registry, :avatar, file_id: upload_3.id, success: false)
           create(:geo_file_registry, :avatar, file_id: upload_4.id)
           create(:geo_file_registry, :avatar, file_id: upload_5.id)
           create(:geo_file_registry, :avatar, file_id: upload_6.id, success: false)
+          create(:geo_file_registry, :lfs, file_id: lfs_object.id, success: false)
 
           secondary.update_attribute(:namespaces, [synced_group])
 

spec/factories/geo/file_registry.rb

@@ -11,5 +11,18 @@ FactoryGirl.define do
     trait :lfs do
       file_type :lfs
     end
+
+    trait :with_file do
+      after(:build, :stub) do |registry, _|
+        file =
+          if registry.file_type == :lfs
+            create(:lfs_object)
+          else
+            create(:upload)
+          end
+
+        registry.file_id = file.id
+      end
+    end
   end
 end

spec/models/geo_node_spec.rb

@@ -326,40 +326,4 @@ describe GeoNode, type: :model do
       end
     end
   end
-
-  describe '#attachments_synced_count' do
-    context 'primary node' do
-      subject { primary_node }
-
-      it 'returns nil' do
-        expect(subject.attachments_synced_count).to be_nil
-      end
-    end
-
-    context 'secondary node' do
-      subject { node }
-
-      it 'returns a value' do
-        expect(subject.attachments_synced_count).to eq(0)
-      end
-    end
-  end
-
-  describe '#attachments_failed_count' do
-    context 'primary node' do
-      subject { primary_node }
-
-      it 'returns nil' do
-        expect(subject.attachments_failed_count).to be_nil
-      end
-    end
-
-    context 'secondary node' do
-      subject { node }
-
-      it 'returns a value' do
-        expect(subject.attachments_failed_count).to eq(0)
-      end
-    end
-  end
 end

spec/models/geo_node_status_spec.rb

 require 'spec_helper'
 
-describe GeoNodeStatus, :geo do
+# Disable transactions via :truncate method because a foreign table
+# can't see changes inside a transaction of a different connection.
+describe GeoNodeStatus, :geo, :truncate do
   include ::EE::GeoHelpers
 
-  set(:primary)  { create(:geo_node, :primary) }
-  set(:secondary) { create(:geo_node) }
+  let!(:primary)  { create(:geo_node, :primary) }
+  let!(:secondary) { create(:geo_node) }
 
-  set(:group)     { create(:group) }
-  set(:project_1) { create(:project, group: group) }
-  set(:project_2) { create(:project, group: group) }
-  set(:project_3) { create(:project) }
-  set(:project_4) { create(:project) }
+  let!(:group)     { create(:group) }
+  let!(:project_1) { create(:project, group: group) }
+  let!(:project_2) { create(:project, group: group) }
+  let!(:project_3) { create(:project) }
+  let!(:project_4) { create(:project) }
 
   subject { described_class.current_node_status }
 
@@ -60,6 +62,7 @@ describe GeoNodeStatus, :geo do
       create(:geo_file_registry, :avatar, file_id: uploads[2], success: false)
 
       expect(subject.attachments_synced_count).to eq(2)
+
     end
 
     it 'does not count synced files that were replaced' do
@@ -96,13 +99,13 @@ describe GeoNodeStatus, :geo do
   describe '#attachments_failed_count' do
     it 'counts failed avatars, attachment, personal snippets and files' do
       # These two should be ignored
-      create(:geo_file_registry, :lfs, success: false)
-      create(:geo_file_registry)
+      create(:geo_file_registry, :lfs, :with_file, success: false)
+      create(:geo_file_registry, :with_file)
 
-      create(:geo_file_registry, file_type: :personal_file, success: false)
-      create(:geo_file_registry, file_type: :attachment, success: false)
-      create(:geo_file_registry, :avatar, success: false)
-      create(:geo_file_registry, success: false)
+      create(:geo_file_registry, :with_file, file_type: :personal_file, success: false)
+      create(:geo_file_registry, :with_file, file_type: :attachment, success: false)
+      create(:geo_file_registry, :avatar, :with_file, success: false)
+      create(:geo_file_registry, :with_file, success: false)
 
       expect(subject.attachments_failed_count).to eq(4)
     end