Improve queries, refactor view and controller, update locale and specs

ee/app/assets/stylesheets/pages/geo_nodes.scss

@@ -221,6 +221,7 @@
         color: $gl-text-red;
       }
     }
+
     @include media-breakpoint-down(xs) {
       .project-status-container + .project-status-container {
         margin-top: 10px;

ee/app/controllers/admin/geo_projects_controller.rb

@@ -9,20 +9,16 @@ class Admin::GeoProjectsController < Admin::ApplicationController
   def index
     finder = ::Geo::ProjectRegistryStatusFinder.new
 
-    case params[:sync_status]
-    when 'never'
-      # This method uses FDW heavily and due to optimizations we need to inject the pagination
-      # earlier as well, so we need the block to do that
-      @projects = finder.never_synced_projects do |fdw_relation|
-        fdw_relation.page(params[:page])
-      end.page(params[:page])
-    when 'failed'
-      @registries = finder.failed_projects.page(params[:page])
-    when 'pending'
-      @registries = finder.pending_projects.page(params[:page])
-    else
-      @registries = finder.synced_projects.page(params[:page])
-    end
+    @registries = case params[:sync_status]
+                  when 'never'
+                    finder.never_synced_projects.page(params[:page])
+                  when 'failed'
+                    finder.failed_projects.page(params[:page])
+                  when 'pending'
+                    finder.pending_projects.page(params[:page])
+                  else
+                    finder.synced_projects.page(params[:page])
+                  end
   end
 
   def recheck

ee/app/finders/geo/project_registry_status_finder.rb

@@ -20,7 +20,7 @@ module Geo
         no_repository_resync
           .and(no_repository_sync_failure)
           .and(repository_verified)
-      ).includes(:project)
+      ).includes(project: :route)
     end
 
     # Return any project registry which project is pending to update
@@ -40,7 +40,7 @@ module Geo
           .and(flagged_for_resync
             .or(repository_pending_verification
               .and(repository_without_verification_failure_before)))
-      ).includes(:project)
+      ).includes(project: :route)
     end
 
     # Return any project registry which project has a failure
@@ -55,31 +55,15 @@ module Geo
         repository_sync_failed
           .or(repository_verification_failed)
           .or(repository_checksum_mismatch)
-      ).includes(:project)
+      ).includes(project: :route)
     end
 
-    # Return projects that has never been fully synced
+    # Return any project registry that has never been fully synced
     #
-    # We include here both projects without a corresponding ProjectRegistry
-    # or projects that have never successfully synced.
-    #
-    # @yield [ActiveRecord::Relation]
-    # @return [Geo::Fdw::Project] Projects that has never been fully synced
+    # We don't include projects without a corresponding ProjectRegistry
+    # for performance reasons.
     def never_synced_projects
-      no_project_registry = project_registry[:project_id].eq(nil)
-      no_repository_synced = project_registry[:last_repository_successful_sync_at].eq(nil)
-
-      project_ids = Geo::Fdw::Project.select(:id).joins("LEFT OUTER JOIN project_registry ON (project_registry.project_id = #{Geo::Fdw::Project.table_name}.id)")
-        .where(
-          no_project_registry
-            .or(no_repository_synced))
-
-      # This allows us to inject pagination
-      if block_given?
-        project_ids = yield(project_ids)
-      end
-
-      Project.where(id: project_ids.to_a).includes(:project_registry)
+      Geo::ProjectRegistry.where(last_repository_successful_sync_at: nil).includes(project: :route)
     end
 
     private

ee/app/models/geo/fdw/project.rb

@@ -2,8 +2,6 @@ module Geo
   module Fdw
     class Project < ::Geo::BaseFdw
       self.table_name = Gitlab::Geo::Fdw.table('projects')
-
-      has_one :project_registry, class_name: 'Geo::ProjectRegistry'
     end
   end
 end

ee/app/models/geo/project_registry.rb

@@ -168,6 +168,18 @@ class Geo::ProjectRegistry < Geo::BaseRegistry
     project.wiki_enabled? && (never_synced_wiki? || wiki_sync_needed?(scheduled_time))
   end
 
+  def repository_verification_pending?
+    self.repository_verification_checksum_sha.nil?
+  end
+
+  def wiki_verification_pending?
+    self.wiki_verification_checksum_sha.nil?
+  end
+
+  def verification_pending?
+    repository_verification_pending? || wiki_verification_pending?
+  end
+
   def syncs_since_gc
     Gitlab::Redis::SharedState.with { |redis| redis.get(fetches_since_gc_redis_key).to_i }
   end
@@ -203,7 +215,7 @@ class Geo::ProjectRegistry < Geo::BaseRegistry
 
   # Flag the repository to be re-checked
   def flag_repository_for_recheck!
-    self.update(repository_verification_checksum_sha: nil, last_repository_verification_failure: nil, repository_checksum_mismatch: nil)
+    self.update(repository_verification_checksum_sha: nil, last_repository_verification_failure: nil, repository_checksum_mismatch: false)
   end
 
   # Flag the repository to be re-synced

ee/app/views/admin/geo_projects/_never.html.haml

-- @projects.each do |project|
+- @registries.each do |project_registry|
   .card.project-card.prepend-top-15
     .card-header{ id: "project-#{project.id}-header" }
       .d-flex
         %strong.header-text-primary.flex-fill
-          = project.full_name
+          = project_registry.project.full_name
 
     .card-body
       .container
@@ -12,30 +12,30 @@
             .project-status-title.text-muted
               = s_('Geo|Next sync scheduled at')
             .project-status-content
-              - if project.project_registry&.repository_retry_at
-                = distance_of_time_in_words(Time.now, project.project_registry.repository_retry_at)
+              - if project_registry.repository_retry_at
+                = distance_of_time_in_words(Time.now, project_registry.repository_retry_at)
               - else
                 = s_('Geo|Never')
           .col-sm.project-status-container
             .project-status-title.text-muted
               = s_('Geo|Last sync attempt')
             .project-status-content
-              - if project.project_registry&.last_repository_synced_at
-                = distance_of_time_in_words(Time.now, project.project_registry.last_repository_synced_at)
+              - if project_registry.last_repository_synced_at
+                = distance_of_time_in_words(Time.now, project_registry.last_repository_synced_at)
               - else
                 = s_('Geo|Never')
           .col-sm.project-status-container
             .project-status-title.text-muted
               = s_('Geo|Retry counts')
             .project-status-content
-              = project.project_registry.repository_retry_count if project.project_registry
+              = project_registry.repository_retry_count
           .col-sm.project-status-container
             .project-status-title.text-muted
               = s_('Geo|Error message')
             .project-status-content.font-weight-bold
-              - if project.project_registry
-                = project.project_registry.last_repository_sync_failure
+              - if project_registry
+                = project_registry.last_repository_sync_failure
               - else
                 = s_('Geo|No errors')
 
-= paginate @projects, theme: 'gitlab'
+= paginate @registries, theme: 'gitlab'

ee/app/views/admin/geo_projects/_pending.html.haml

@@ -24,7 +24,7 @@
               - if project_registry.repository_retry_at
                 = distance_of_time_in_words(Time.now, project_registry.repository_retry_at)
               - else
-              = s_('Geo|Never')
+                = s_('Geo|Never')
           .col-sm.project-status-container
             .project-status-title.text-muted
               = s_('Geo|Last sync attempt')

ee/app/views/admin/geo_projects/index.html.haml

@@ -28,4 +28,4 @@
   - when 'pending'
     = render(partial: 'pending')
   - else
-    = render(partial: 'all')
+    = render(partial: 'synced')

ee/spec/controllers/admin/geo_projects_controller_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Admin::GeoProjectsController, :geo do
+  set(:admin) { create(:admin) }
+  let(:synced_registry) { create(:geo_project_registry, :synced) }
+
+  before do
+    sign_in(admin)
+  end
+
+  shared_examples 'license required' do
+    context 'without a valid license' do
+      it 'redirects to license page with a flash message' do
+        expect(subject).to redirect_to(admin_license_path)
+        expect(flash[:alert]).to include('You need a different license to use Geo replication')
+      end
+    end
+  end
+
+  describe '#index' do
+    subject { get :index }
+
+    it_behaves_like 'license required'
+
+    context 'with a valid license' do
+      before do
+        allow(Gitlab::Geo).to receive(:license_allows?).and_return(true)
+      end
+
+      it 'renders synced template when no extra get params is specified' do
+        expect(subject).to have_gitlab_http_status(200)
+        expect(subject).to render_template(:index, partial: :synced)
+      end
+
+      context 'with sync_status=pending' do
+        subject { get :index, sync_status: 'pending' }
+
+        it 'renders pending template' do
+          expect(subject).to have_gitlab_http_status(200)
+          expect(subject).to render_template(:index, partial: :pending)
+        end
+      end
+
+      context 'with sync_status=failed' do
+        subject { get :index, sync_status: 'failed' }
+
+        it 'renders failed template' do
+          expect(subject).to have_gitlab_http_status(200)
+          expect(subject).to render_template(:index, partial: :failed)
+        end
+      end
+
+      context 'with sync_status=never' do
+        subject { get :index, sync_status: 'never' }
+
+        it 'renders failed template' do
+          expect(subject).to have_gitlab_http_status(200)
+          expect(subject).to render_template(:index, partial: :never)
+        end
+      end
+    end
+  end
+
+  describe '#recheck' do
+    subject { post :recheck, id: synced_registry }
+
+    it_behaves_like 'license required'
+
+    context 'with a valid license' do
+      before do
+        allow(Gitlab::Geo).to receive(:license_allows?).and_return(true)
+      end
+
+      it 'flags registry for recheck' do
+        expect(subject).to redirect_to(admin_geo_projects_path)
+        expect(flash[:notice]).to include('is scheduled for re-check')
+        expect(synced_registry.reload.verification_pending?).to be_truthy
+      end
+    end
+  end
+
+  describe '#resync' do
+    subject { post :resync, id: synced_registry }
+
+    it_behaves_like 'license required'
+
+    context 'with a valid license' do
+      before do
+        allow(Gitlab::Geo).to receive(:license_allows?).and_return(true)
+      end
+
+      it 'flags registry for resync' do
+        expect(subject).to redirect_to(admin_geo_projects_path)
+        expect(flash[:notice]).to include('is scheduled for re-sync')
+        expect(synced_registry.reload.resync_repository?).to be_truthy
+      end
+    end
+  end
+
+  describe '#force_redownload' do
+    subject { post :force_redownload, id: synced_registry }
+
+    it_behaves_like 'license required'
+
+    context 'with a valid license' do
+      before do
+        allow(Gitlab::Geo).to receive(:license_allows?).and_return(true)
+      end
+
+      it 'flags registry for re-download' do
+        expect(subject).to redirect_to(admin_geo_projects_path)
+        expect(flash[:notice]).to include('is scheduled for forced re-download')
+        expect(synced_registry.reload.should_be_redownloaded?('repository')).to be_truthy
+      end
+    end
+  end
+end

ee/spec/finders/geo/project_registry_status_finder_spec.rb

@@ -17,8 +17,6 @@ describe Geo::ProjectRegistryStatusFinder, :geo do
 
   set(:never_synced_registry) { create(:geo_project_registry) }
   set(:never_synced_registry_with_failure) { create(:geo_project_registry, :repository_sync_failed) }
-  set(:project_without_registry) { create(:project, name: 'project without registry') }
-  let(:project_with_never_synced_registry) { never_synced_registry.project }
 
   subject { described_class.new(current_node: secondary) }
 
@@ -61,13 +59,12 @@ describe Geo::ProjectRegistryStatusFinder, :geo do
   end
 
   describe '#never_synced_projects' do
-    it 'returns only FDW projects without registry or with never synced registries' do
+    it 'returns only never fully synced registries' do
       result = subject.never_synced_projects
 
       expect(result).to contain_exactly(
-        project_without_registry,
-        project_with_never_synced_registry,
-        never_synced_registry_with_failure.project
+        never_synced_registry,
+        never_synced_registry_with_failure
       )
     end
   end

locale/gitlab.pot

@@ -3262,6 +3262,9 @@ msgstr ""
 msgid "Geo|Next sync scheduled at"
 msgstr ""
 
+msgid "Geo|No errors"
+msgstr ""
+
 msgid "Geo|Pending"
 msgstr ""