Does not raises an error 500 for orphaned entries

This changes guard against orphaned Geo::ProjectRegistry
entries that projects that have been deleted in the Geo
project list page. It also allows administrators to remove
those orphaned entries through UI.

config/routes/admin.rb

@@ -135,7 +135,7 @@ namespace :admin do
   namespace :geo do
     resources :nodes, only: [:index, :create, :new, :edit, :update]
 
-    resources :projects, only: :index do
+    resources :projects, only: [:index, :destroy] do
       member do
         post :recheck
         post :resync

ee/app/controllers/admin/geo/projects_controller.rb

@@ -21,6 +21,16 @@ class Admin::Geo::ProjectsController < Admin::ApplicationController
                   end
   end
 
+  def destroy
+    unless @registry.project.nil?
+      return redirect_back_or_admin_geo_projects(alert: s_('Geo|Could not remove tracking entry for an existing project.'))
+    end
+
+    @registry.destroy
+
+    redirect_back_or_admin_geo_projects(notice: s_('Geo|Tracking entry for project (%{project_id}) was successfully removed.') % { project_id: @registry.project_id })
+  end
+
   def recheck
     @registry.flag_repository_for_recheck!
 

ee/app/views/admin/geo/projects/_failed.html.haml

 - @registries.each do |project_registry|
   .card.project-card.prepend-top-15
-    .card-header{ id: "project-#{project_registry.project.id}-header" }
+    .card-header{ id: "project-#{project_registry.project_id}-header" }
       .d-flex
-        %strong.header-text-primary.flex-fill
-          = link_to project_registry.project.full_name, admin_namespace_project_path(project_registry.project.namespace, project_registry.project)
-        - if project_registry.candidate_for_redownload?
-          = link_to(force_redownload_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline btn-sm mr-2') do
-            = s_('Geo|Redownload')
-        = link_to(resync_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline-primary btn-sm') do
-          = s_('Geo|Resync')
+        - if project_registry.project.nil?
+          = render partial: 'removed', locals: { project_registry: project_registry }
+        - else
+          %strong.header-text-primary.flex-fill
+            = link_to project_registry.project.full_name, admin_namespace_project_path(project_registry.project.namespace, project_registry.project)
+          - if project_registry.candidate_for_redownload?
+            = link_to(force_redownload_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline btn-sm mr-2') do
+              = s_('Geo|Redownload')
+          = link_to(resync_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline-primary btn-sm') do
+            = s_('Geo|Resync')
 
     .card-body
       .container.project-container
@@ -43,15 +46,15 @@
     .project-card-errors
       .card-header.bg-transparent.border-bottom-0.border-top
         %button.btn.btn-link.btn-card-header.collapsed.d-flex{ type: 'button',
-          data: { toggle: 'collapse', target: "#project-errors-#{project_registry.project.id}" },
+          data: { toggle: 'collapse', target: "#project-errors-#{project_registry.project_id}" },
           'aria-expanded' => 'false',
-          'aria-controls' => "project-errors-#{project_registry.project.id}" }
+          'aria-controls' => "project-errors-#{project_registry.project_id}" }
           = sprite_icon('chevron-down', size: 18, css_class: 'append-right-5 card-expand-icon')
           = sprite_icon('chevron-up', size: 18, css_class: 'append-right-5 card-collapse-icon')
           .header-text-secondary
             More
-      .collapse{ id: "project-errors-#{project_registry.project.id}",
-        'aria-labelledby' => "project-#{project_registry.project.id}-header" }
+      .collapse{ id: "project-errors-#{project_registry.project_id}",
+        'aria-labelledby' => "project-#{project_registry.project_id}-header" }
         .card-body
           .container.project-container
             %ul.unstyled-list.errors-list

ee/app/views/admin/geo/projects/_never.html.haml

 - @registries.each do |project_registry|
   .card.project-card.prepend-top-15
-    .card-header{ id: "project-#{project_registry.project.id}-header" }
+    .card-header{ id: "project-#{project_registry.project_id}-header" }
       .d-flex
-        %strong.header-text-primary.flex-fill
-          = link_to project_registry.project.full_name, admin_namespace_project_path(project_registry.project.namespace, project_registry.project)
+        - if project_registry.project.nil?
+          = render partial: 'removed', locals: { project_registry: project_registry }
+        - else
+          %strong.header-text-primary.flex-fill
+            = link_to project_registry.project.full_name, admin_namespace_project_path(project_registry.project.namespace, project_registry.project)
 
     .card-body
       .container.project-container

ee/app/views/admin/geo/projects/_pending.html.haml

 - @registries.each do |project_registry|
   .card.project-card.prepend-top-15
-    .card-header{ id: "project-#{project_registry.project.id}-header" }
+    .card-header{ id: "project-#{project_registry.project_id}-header" }
       .d-flex
-        %strong.header-text-primary.flex-fill
-          = link_to project_registry.project.full_name, admin_namespace_project_path(project_registry.project.namespace, project_registry.project)
-        - unless project_registry.verification_pending?
-          = link_to(recheck_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline btn-sm mr-2') do
-            = s_('Geo|Recheck')
-        - unless project_registry.resync_repository?
-          = link_to(resync_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline-primary btn-sm') do
-            = s_('Geo|Resync')
+        - if project_registry.project.nil?
+          = render partial: 'removed', locals: { project_registry: project_registry }
+        - else
+          %strong.header-text-primary.flex-fill
+            = link_to project_registry.project.full_name, admin_namespace_project_path(project_registry.project.namespace, project_registry.project)
+          - unless project_registry.verification_pending?
+            = link_to(recheck_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline btn-sm mr-2') do
+              = s_('Geo|Recheck')
+          - unless project_registry.resync_repository?
+            = link_to(resync_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline-primary btn-sm') do
+              = s_('Geo|Resync')
 
     .card-body
       .container.project-container

ee/app/views/admin/geo/projects/_removed.html.haml

+%strong.header-text-primary.flex-fill
+  = s_('Geo|Project (ID: %{project_id}) no longer exists on the primary. It is safe to remove this entry, as this will not remove any data on disk.') % { project_id: project_registry.project_id }
+= link_to(admin_geo_project_path(project_registry), data: { confirm: s_('Geo|Tracking entry will be removed. Are you sure?')}, method: :delete, class: 'btn btn-inverted btn-remove btn-sm') do
+  = s_('Geo|Remove')

ee/app/views/admin/geo/projects/_synced.html.haml

 - @registries.each do |project_registry|
   .card.project-card.prepend-top-15
-    .card-header{ id: "project-#{project_registry.project.id}-header" }
+    .card-header{ id: "project-#{project_registry.project_id}-header" }
       .d-flex
-        %strong.header-text-primary.flex-fill
-          = link_to project_registry.project.full_name, admin_namespace_project_path(project_registry.project.namespace, project_registry.project)
-        = link_to(recheck_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline btn-sm mr-2') do
-          = s_('Geo|Recheck')
-        = link_to(resync_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline-primary btn-sm') do
-          = s_('Geo|Resync')
+        - if project_registry.project.nil?
+          = render partial: 'removed', locals: { project_registry: project_registry }
+        - else
+          %strong.header-text-primary.flex-fill
+            = link_to project_registry.project.full_name, admin_namespace_project_path(project_registry.project.namespace, project_registry.project)
+          = link_to(recheck_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline btn-sm mr-2') do
+            = s_('Geo|Recheck')
+          = link_to(resync_admin_geo_project_path(project_registry), method: :post, class: 'btn btn-outline-primary btn-sm') do
+            = s_('Geo|Resync')
 
     .card-body
       .container.project-container

ee/changelogs/unreleased/7706-geo-500-returned-for-admin-geo_projects-when-no-projects-exist.yml

+---
+title: Geo - Does not raise error 500 on Geo projects list page for orphaned entries
+merge_request: 7565
+author:
+type: fixed

ee/lib/ee/gitlab/middleware/read_only/controller.rb

@@ -10,7 +10,7 @@ module EE
           }.freeze
 
           WHITELISTED_GEO_ROUTES_TRACKING_DB = {
-            'admin/geo/projects' => %w{resync recheck force_redownload}
+            'admin/geo/projects' => %w{destroy resync recheck force_redownload}
           }.freeze
 
           private

ee/spec/controllers/admin/geo/projects_controller_spec.rb

@@ -62,6 +62,36 @@ describe Admin::Geo::ProjectsController, :geo do
     end
   end
 
+  describe '#destroy' do
+    subject { delete :destroy, 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
+
+      context 'with an orphaned registry' do
+        it 'removes the registry' do
+          synced_registry.update_column(:project_id, -1)
+
+          expect(subject).to redirect_to(admin_geo_projects_path)
+          expect(flash[:notice]).to include('was successfully removed')
+          expect { Geo::ProjectRegistry.find(synced_registry.id) }.to raise_error(ActiveRecord::RecordNotFound)
+        end
+      end
+
+      context 'with a regular registry' do
+        it 'removes the registry' do
+          expect(subject).to redirect_to(admin_geo_projects_path)
+          expect(flash[:alert]).to include('Could not remove tracking entry')
+          expect { Geo::ProjectRegistry.find(synced_registry.id) }.not_to raise_error
+        end
+      end
+    end
+  end
+
   describe '#recheck' do
     subject { post :recheck, id: synced_registry }
 

ee/spec/lib/ee/gitlab/middleware/read_only_spec.rb

@@ -49,6 +49,38 @@ describe Gitlab::Middleware::ReadOnly do
         expect(response).not_to be_redirect
         expect(subject).not_to disallow_request
       end
+
+      it 'expects a DELETE request to geo projects delete URL to be allowed' do
+        expect(Rails.application.routes).to receive(:recognize_path).and_call_original
+        response = request.delete('/admin/geo/projects/1')
+
+        expect(response).not_to be_redirect
+        expect(subject).not_to disallow_request
+      end
+
+      it 'expects a POST request to geo projects resync URL to be allowed' do
+        expect(Rails.application.routes).to receive(:recognize_path).and_call_original
+        response = request.post('/admin/geo/projects/1/resync')
+
+        expect(response).not_to be_redirect
+        expect(subject).not_to disallow_request
+      end
+
+      it 'expects a POST request to geo projects recheck URL to be allowed' do
+        expect(Rails.application.routes).to receive(:recognize_path).and_call_original
+        response = request.post('/admin/geo/projects/1/recheck')
+
+        expect(response).not_to be_redirect
+        expect(subject).not_to disallow_request
+      end
+
+      it 'expects a POST request to geo projects force redownload URL to be allowed' do
+        expect(Rails.application.routes).to receive(:recognize_path).and_call_original
+        response = request.post('/admin/geo/projects/1/force_redownload')
+
+        expect(response).not_to be_redirect
+        expect(subject).not_to disallow_request
+      end
     end
   end
 end

ee/spec/routing/admin_routing_spec.rb

@@ -9,15 +9,19 @@ describe 'EE-specific admin routing' do
       expect(get('/admin/geo/projects')).to route_to('admin/geo/projects#index')
     end
 
-    it 'routes /:id/recheck to #recheck' do
+    it 'routes delete /:id to #destroy' do
+      expect(delete("/admin/geo/projects/#{project_registry.id}")).to route_to('admin/geo/projects#destroy', id: project_registry.to_param)
+    end
+
+    it 'routes post /:id/recheck to #recheck' do
       expect(post("admin/geo/projects/#{project_registry.id}/recheck")).to route_to('admin/geo/projects#recheck', id: project_registry.to_param)
     end
 
-    it 'routes /id:/resync to #resync' do
+    it 'routes post /:id/resync to #resync' do
       expect(post("admin/geo/projects/#{project_registry.id}/resync")).to route_to('admin/geo/projects#resync', id: project_registry.to_param)
     end
 
-    it 'routes /id:/force_redownload to #force_redownload' do
+    it 'routes post /:id/force_redownload to #force_redownload' do
       expect(post("admin/geo/projects/#{project_registry.id}/force_redownload")).to route_to('admin/geo/projects#force_redownload', id: project_registry.to_param)
     end
   end

locale/gitlab.pot

@@ -3530,6 +3530,9 @@ msgstr ""
 msgid "Geo|All projects"
 msgstr ""
 
+msgid "Geo|Could not remove tracking entry for an existing project."
+msgstr ""
+
 msgid "Geo|Error message"
 msgstr ""
 
@@ -3572,6 +3575,9 @@ msgstr ""
 msgid "Geo|Pending verification"
 msgstr ""
 
+msgid "Geo|Project (ID: %{project_id}) no longer exists on the primary. It is safe to remove this entry, as this will not remove any data on disk."
+msgstr ""
+
 msgid "Geo|Projects in certain groups"
 msgstr ""
 
@@ -3584,6 +3590,9 @@ msgstr ""
 msgid "Geo|Redownload"
 msgstr ""
 
+msgid "Geo|Remove"
+msgstr ""
+
 msgid "Geo|Repository sync capacity"
 msgstr ""
 
@@ -3611,6 +3620,12 @@ msgstr ""
 msgid "Geo|Synchronization failed - %{error}"
 msgstr ""
 
+msgid "Geo|Tracking entry for project (%{project_id}) was successfully removed."
+msgstr ""
+
+msgid "Geo|Tracking entry will be removed. Are you sure?"
+msgstr ""
+
 msgid "Geo|Unknown state"
 msgstr ""