Merge branch '31938-move-geo-settings' into 'master'

Redirect Admin > Settings > Geo TO Admin > Geo > Settings

Closes #31938

See merge request gitlab-org/gitlab!19833

app/views/layouts/nav/sidebar/_admin.html.haml

@@ -264,8 +264,8 @@
               = link_to network_admin_application_settings_path, title: _('Network'), data: { qa_selector: 'admin_settings_network_item' } do
                 %span
                   = _('Network')
-            - if template_exists?('admin/application_settings/geo')
-              = nav_link(path: 'application_settings#geo') do
+            - if template_exists?('admin/geo/settings/show')
+              = nav_link do
                 = link_to geo_admin_application_settings_path, title: _('Geo') do
                   %span
                     = _('Geo')

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

+# frozen_string_literal: true
+
+class Admin::Geo::SettingsController < Admin::ApplicationSettingsController
+  helper ::EE::GeoHelper
+  before_action :check_license!
+
+  def show
+  end
+
+  protected
+
+  def check_license!
+    unless Gitlab::Geo.license_allows?
+      flash[:alert] = _('You need a different license to use Geo replication.')
+      redirect_to admin_license_path
+    end
+  end
+end

ee/app/controllers/ee/admin/application_settings_controller.rb

@@ -5,7 +5,7 @@ module EE
     module ApplicationSettingsController
       extend ::Gitlab::Utils::Override
 
-      EE_VALID_SETTING_PANELS = %w(geo templates).freeze
+      EE_VALID_SETTING_PANELS = %w(templates).freeze
 
       EE_VALID_SETTING_PANELS.each do |action|
         define_method(action) { perform_update if submitted? }
@@ -45,6 +45,11 @@ module EE
         attrs
       end
 
+      def geo_redirection
+        redirect_to admin_geo_settings_url, notice: 'You were automatically redirected to <strong>Admin Area > Geo > Setting</strong><br /> '\
+                                                    'From GitLab 12.7 on, this will be the only place for Geo settings and <strong>Admin Area > Settings > Geo</strong> will be removed.'.html_safe
+      end
+
       private
 
       override :valid_setting_panels

ee/app/views/admin/application_settings/_geo.html.haml

-- if Gitlab::Geo.license_allows?
-  %section.settings.as-geo.no-animate#js-geo-settings{ class: ('expanded' if expanded) }
-    .settings-header
-      %h4
-        = _('GitLab Geo')
-      %button.btn.js-settings-toggle{ type: 'button' }
-        = expanded ? 'Collapse' : 'Expand'
-      %p
-        = _('Geo allows you to replicate your GitLab instance to other geographical locations.')
-    .settings-content
-
-      = form_for @application_setting, url: geo_admin_application_settings_path(anchor: 'js-geo-settings'), html: { class: 'fieldset-form' } do |f|
-        = form_errors(@application_setting)
-
-        %fieldset
-          .form-group
-            = f.label :geo_status_timeout, 'Connection timeout', class: 'label-bold'
-            = f.number_field :geo_status_timeout, class: 'form-control'
-            .form-text.text-muted
-              = _('The amount of seconds after which a request to get a secondary node status will time out.')
-          .form-group
-            = f.label :geo_node_allowed_ips, 'Allowed Geo IP', class: 'label-bold'
-            = f.text_field :geo_node_allowed_ips, class: 'form-control'
-            .form-text.text-muted
-              = _('List of IPs and CIDRs of allowed secondary nodes. Comma-separated, e.g. "1.1.1.1, 2.2.2.0/24"')
-
-        = f.submit _('Save changes'), class: "btn btn-success"
-- else
-  = render 'shared/empty_states/geo'

ee/app/views/admin/application_settings/geo.html.haml

-- breadcrumb_title "Geo"
-- page_title "Geo"
-- @content_class = "limit-container-width" unless fluid_layout
-
-= render_if_exists 'admin/application_settings/geo', expanded: expanded_by_default?

ee/app/views/admin/geo/settings/_form.html.haml

+- if Gitlab::Geo.license_allows?
+  %section
+    .settings-header
+      %h4
+        = _('Geo Settings')
+      %p
+        = _('Geo allows you to replicate your GitLab instance to other geographical locations.')
+    = form_for @application_setting, url: admin_geo_settings_path, html: { class: 'fieldset-form' } do |f|
+      = form_errors(@application_setting)
+      %fieldset
+        .form-group
+          = f.label :geo_status_timeout, 'Connection timeout', class: 'label-bold'
+          = f.number_field :geo_status_timeout, class: 'form-control'
+          .form-text.text-muted
+            = _('The amount of seconds after which a request to get a secondary node status will time out.')
+        .form-group
+          = f.label :geo_node_allowed_ips, 'Allowed Geo IP', class: 'label-bold'
+          = f.text_field :geo_node_allowed_ips, class: 'form-control'
+          .form-text.text-muted
+            = _('List of IPs and CIDRs of allowed secondary nodes. Comma-separated, e.g. "1.1.1.1, 2.2.2.0/24"')
+
+      = f.submit _('Save changes'), class: "btn btn-success"
+- else
+  = render 'shared/empty_states/geo'

ee/app/views/admin/geo/settings/show.html.haml

+- page_title "Geo Settings"
+- @content_class = "limit-container-width geo-admin-container geo-admin-settings" unless fluid_layout
+
+= render_if_exists 'admin/geo/settings/form'

ee/app/views/layouts/nav/ee/admin/_geo_sidebar.html.haml

-= nav_link(controller: %w(admin/geo/nodes admin/geo/projects admin/geo/uploads)) do
+= nav_link(controller: %w(admin/geo/nodes admin/geo/projects admin/geo/uploads admin/geo/settings)) do
   = link_to admin_geo_nodes_path, class: "qa-link-geo-menu" do
     .nav-icon-container
       = sprite_icon('location-dot')
@@ -9,12 +9,15 @@
       = link_to admin_geo_nodes_path do
         %strong.fly-out-top-item-name
           #{ _('Geo') }
-    - if Gitlab::Geo.secondary?
-      %li.divider.fly-out-top-item
       = nav_link(path: 'admin/geo/nodes#index') do
         = link_to admin_geo_nodes_path, title: 'Nodes' do
           %span
-            #{ _('Nodes') }
+            = _('Nodes')
+      = nav_link(path: 'admin/geo/settings#show') do
+        = link_to admin_geo_settings_path, title: 'Settings' do
+          %span
+            = _('Settings')
+    - if Gitlab::Geo.secondary?
       = nav_link(path: 'admin/geo/projects#index') do
         = link_to admin_geo_projects_path, title: 'Projects' do
           %span

ee/changelogs/unreleased/31938-move-geo-settings.yml

+---
+title: Redirect Admin > Settings > Geo to Admin > Geo > Settings
+merge_request: 19833
+author: 
+type: changed

ee/config/routes/admin.rb

@@ -27,7 +27,8 @@ namespace :admin do
 
   # using `only: []` to keep duplicate routes from being created
   resource :application_settings, only: [] do
-    match :geo, :templates, via: [:get, :patch]
+    match :templates, via: [:get, :patch]
+    get :geo, to: "application_settings#geo_redirection"
   end
 
   namespace :geo do
@@ -48,6 +49,8 @@ namespace :admin do
       end
     end
 
+    resource :settings, only: [:show, :update]
+
     resources :uploads, only: [:index, :destroy]
   end
 

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -140,5 +140,21 @@ describe Admin::ApplicationSettingsController do
         end
       end
     end
+
+    describe 'GET #geo_redirection' do
+      subject { get :geo_redirection }
+
+      it 'redirects the user to the admin_geo_settings_url' do
+        subject
+
+        expect(response).to redirect_to(admin_geo_settings_url)
+      end
+
+      it 'fires a notice about the redirection' do
+        subject
+
+        expect(response).to set_flash[:notice]
+      end
+    end
   end
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Admin::Geo::SettingsController, :geo do
+  include EE::GeoHelpers
+  include StubENV
+
+  set(:admin) { create(:admin) }
+
+  before do
+    stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+  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 '#show' do
+    before do
+      sign_in(admin)
+    end
+
+    subject { get :show }
+
+    it_behaves_like 'license required'
+
+    context 'with a valid license' do
+      render_views
+
+      before do
+        stub_licensed_features(geo: true)
+      end
+
+      it 'renders the show template' do
+        expect(subject).to have_gitlab_http_status(200)
+        expect(subject).to render_template(:show)
+      end
+    end
+  end
+
+  describe '#update' do
+    before do
+      sign_in(admin)
+    end
+
+    String test_value = '1.0.0.0/0, ::/0'
+
+    context 'with a valid license' do
+      before do
+        stub_licensed_features(geo: true)
+        @request.env['HTTP_REFERER'] = admin_geo_settings_path
+        patch :update, params: { application_setting: { geo_node_allowed_ips: test_value } }
+      end
+
+      it 'sets the geo node property in ApplicationSetting' do
+        expect(ApplicationSetting.current.geo_node_allowed_ips).to eq(test_value)
+      end
+
+      it 'redirects the update to the referer' do
+        expect(request).to redirect_to(admin_geo_settings_path)
+      end
+    end
+  end
+end

ee/spec/features/admin/admin_settings_spec.rb

@@ -14,8 +14,8 @@ describe 'Admin updates EE-only settings' do
   context 'Geo settings' do
     context 'when the license has Geo feature' do
       it 'allows users to change Geo settings' do
-        visit geo_admin_application_settings_path
-        page.within('.as-geo') do
+        visit admin_geo_settings_path
+        page.within('section') do
           fill_in 'Connection timeout', with: 15
           fill_in 'Allowed Geo IP', with: '192.34.34.34'
           click_button 'Save changes'
@@ -31,9 +31,9 @@ describe 'Admin updates EE-only settings' do
       it 'shows empty page' do
         allow(License).to receive(:feature_available?).and_return(false)
 
-        visit geo_admin_application_settings_path
+        visit admin_geo_settings_path
 
-        expect(page).to have_content 'Discover GitLab Geo'
+        expect(page).to have_content 'You need a different license to use Geo replication'
       end
     end
   end

ee/spec/routing/admin_routing_spec.rb

@@ -50,6 +50,16 @@ describe 'EE-specific admin routing' do
     end
   end
 
+  describe Admin::Geo::SettingsController, 'routing' do
+    it 'routes / to #show' do
+      expect(get('/admin/geo/settings')).to route_to('admin/geo/settings#show')
+    end
+
+    it 'routes patch / to #update' do
+      expect(patch('/admin/geo/settings')).to route_to('admin/geo/settings#update')
+    end
+  end
+
   describe Admin::EmailsController, 'routing' do
     it 'routes to #show' do
       expect(get('/admin/email')).to route_to('admin/emails#show')
@@ -61,9 +71,8 @@ describe 'EE-specific admin routing' do
   end
 
   describe Admin::ApplicationSettingsController, 'routing' do
-    it 'routes to #geo' do
-      expect(get('/admin/application_settings/geo')).to route_to('admin/application_settings#geo')
-      expect(patch('/admin/application_settings/geo')).to route_to('admin/application_settings#geo')
+    it 'redirects #geo to #geo_redirection' do
+      expect(get('/admin/application_settings/geo')).to route_to('admin/application_settings#geo_redirection')
     end
 
     it 'routes to #templates' do

locale/gitlab.pot

@@ -7764,6 +7764,9 @@ msgstr ""
 msgid "Geo Nodes"
 msgstr ""
 
+msgid "Geo Settings"
+msgstr ""
+
 msgid "Geo allows you to replicate your GitLab instance to other geographical locations."
 msgstr ""
 
@@ -8214,9 +8217,6 @@ msgstr ""
 msgid "GitLab Enterprise Edition %{plan}"
 msgstr ""
 
-msgid "GitLab Geo"
-msgstr ""
-
 msgid "GitLab Group Runners can execute code for all the projects in this group."
 msgstr ""