Merge branch 'sh-add-geo-node-edit' into 'master'

Add ability to edit a GeoNode

Closes #2856

See merge request !2471

app/controllers/admin/geo_nodes_controller.rb

 class Admin::GeoNodesController < Admin::ApplicationController
   before_action :check_license, except: [:index, :destroy]
-  before_action :load_node, only: [:destroy, :repair, :toggle, :status]
+  before_action :load_node, only: [:edit, :update, :destroy, :repair, :toggle, :status]
 
   def index
     @nodes = GeoNode.all.order(:id)
@@ -22,6 +22,14 @@ class Admin::GeoNodesController < Admin::ApplicationController
     end
   end
 
+  def update
+    if @node.update_attributes(geo_node_params.except(:geo_node_key_attributes))
+      redirect_to admin_geo_nodes_path, notice: 'Geo Node was successfully updated.'
+    else
+      render 'edit'
+    end
+  end
+
   def destroy
     @node.destroy
 

app/views/admin/geo_nodes/_form.html.haml

-= form_for geo_node, as: :geo_node, url: admin_geo_nodes_path, html: { class: 'form-horizontal' } do |f|
-  - if geo_node.errors.any?
-    .alert.alert-danger
-      - geo_node.errors.full_messages.each do |msg|
-        %p= msg
+- disable_key_edit = local_assigns.fetch(:disable_key_edit, false)
+
+= form_errors(geo_node)
+.form-group
+  .col-sm-offset-2.col-sm-10
+    .checkbox
+      = form.label :primary do
+        = form.check_box :primary
+        %strong This is a primary node
+.form-group
+  = form.label :url, 'URL', class: 'control-label'
+  .col-sm-10
+    = form.text_field :url, class: 'form-control'
+
+= form.fields_for :geo_node_key, geo_node.geo_node_key, include_id: !disable_key_edit do |fg|
   .form-group
-    .col-sm-offset-2.col-sm-10
-      .checkbox
-        = f.label :primary do
-          = f.check_box :primary
-          %strong This is a primary node
-  .form-group
-    = f.label :url, 'URL', class: 'control-label'
+    = fg.label :key, 'Public Key', class: 'control-label'
     .col-sm-10
-      = f.text_field :url, class: 'form-control'
-  = f.fields_for :geo_node_key, geo_node.geo_node_key do |fg|
-    .form-group
-      = fg.label :key, 'Public Key', class: 'control-label'
-      .col-sm-10
-        = fg.text_area :key, class: 'form-control thin_area', rows: 5
+      = fg.text_area :key, class: 'form-control thin_area', rows: 5, disabled: disable_key_edit
+      - unless disable_key_edit
         %p.help-block
           Paste a machine public key here for the GitLab user this node runs on. Read more about how to generate it
           = link_to "here", help_page_path("ssh/README")
-  .form-actions
-    = f.submit 'Add Node', class: 'btn btn-create'
-%hr

app/views/admin/geo_nodes/edit.html.haml

+- page_title 'Edit Geo Node'
+%h3.page-title
+  Edit Geo Node
+
+= form_for [:admin, @node], html: { class: 'form-horizontal' } do |f|
+  = render partial: 'form', locals: { form: f, geo_node: @node, disable_key_edit: true }
+  .form-actions
+    = f.submit 'Save changes', class: 'btn btn-create'
+    = link_to 'Cancel', admin_geo_nodes_path, class: 'btn btn-cancel'
+
+%hr

app/views/admin/geo_nodes/index.html.haml

@@ -8,7 +8,13 @@
 
 %hr
 
-= render partial: 'form', locals: {geo_node: @node} if Gitlab::Geo.license_allows?
+- if Gitlab::Geo.license_allows?
+  = form_for [:admin, @node], as: :geo_node, url: admin_geo_nodes_path, html: { class: 'form-horizontal' } do |f|
+    = render partial: 'form', locals: { form: f, geo_node: @node }
+    .form-actions
+      = f.submit 'Add Node', class: 'btn btn-create'
+
+%hr
 
 - if @nodes.any?
   .panel.panel-default
@@ -52,11 +58,12 @@
                 %p
                   .js-health
 
-          - if Gitlab::Geo.primary?
+          - unless Gitlab::Geo.secondary?
             .node-actions
               - if Gitlab::Geo.license_allows?
                 - if node.missing_oauth_application?
                   = link_to "Repair authentication", repair_admin_geo_node_path(node), method: :post, title: 'OAuth application is missing', class: 'btn btn-default btn-sm'
                 - if node.secondary?
                   = toggle_node_button(node)
+                = link_to "Edit", edit_admin_geo_node_path(node), class: 'btn btn-sm'
               = link_to "Remove", admin_geo_node_path(node), data: { confirm: 'Are you sure?' }, method: :delete, class: 'btn btn-remove btn-sm'

config/routes/admin.rb

@@ -125,7 +125,7 @@ namespace :admin do
     get :download, on: :member
   end
 
-  resources :geo_nodes, only: [:index, :create, :destroy] do
+  resources :geo_nodes, only: [:index, :create, :edit, :update, :destroy] do
     member do
       post :repair
       post :toggle

spec/controllers/admin/geo_nodes_controller_spec.rb

@@ -20,7 +20,10 @@ describe Admin::GeoNodesController, :postgresql do
 
   describe '#index' do
     render_views
-    subject { get :index }
+
+    def go
+      get :index
+    end
 
     context 'with add-on license available' do
       before do
@@ -28,7 +31,7 @@ describe Admin::GeoNodesController, :postgresql do
       end
 
       it 'renders creation form' do
-        expect(subject).to render_template(partial: 'admin/geo_nodes/_form')
+        expect(go).to render_template(partial: 'admin/geo_nodes/_form')
       end
     end
 
@@ -38,16 +41,16 @@ describe Admin::GeoNodesController, :postgresql do
       end
 
       it 'does not render the creation form' do
-        expect(subject).not_to render_template(partial: 'admin/geo_nodes/_form')
+        expect(go).not_to render_template(partial: 'admin/geo_nodes/_form')
       end
 
       it 'displays a flash message' do
-        subject
+        go
         expect(controller).to set_flash.now[:alert].to('You need a different license to enable Geo replication')
       end
 
       it 'does not redirects to the license page' do
-        subject
+        go
         expect(response).not_to redirect_to(admin_license_path)
       end
     end
@@ -55,7 +58,8 @@ describe Admin::GeoNodesController, :postgresql do
 
   describe '#destroy' do
     let!(:geo_node) { create(:geo_node) }
-    subject do
+
+    def go
       delete(:destroy, id: geo_node)
     end
 
@@ -65,7 +69,7 @@ describe Admin::GeoNodesController, :postgresql do
       end
 
       it 'deletes the node' do
-        expect { subject }.to change { GeoNode.count }.by(-1)
+        expect { go }.to change { GeoNode.count }.by(-1)
       end
     end
 
@@ -75,19 +79,22 @@ describe Admin::GeoNodesController, :postgresql do
       end
 
       it 'deletes the node' do
-        expect { subject }.to change { GeoNode.count }.by(-1)
+        expect { go }.to change { GeoNode.count }.by(-1)
       end
     end
   end
 
   describe '#create' do
     let(:geo_node_attributes) { { url: 'http://example.com', geo_node_key_attributes: { key: SSHKeygen.generate } } }
-    subject { post :create, geo_node: geo_node_attributes }
+
+    def go
+      post :create, geo_node: geo_node_attributes
+    end
 
     context 'without add-on license' do
       before do
         allow(Gitlab::Geo).to receive(:license_allows?) { false }
-        subject
+        go
       end
 
       it_behaves_like 'unlicensed geo action'
@@ -99,18 +106,53 @@ describe Admin::GeoNodesController, :postgresql do
       end
 
       it 'creates the node' do
-        expect { subject }.to change { GeoNode.count }.by(1)
+        expect { go }.to change { GeoNode.count }.by(1)
+      end
+    end
+  end
+
+  describe '#update' do
+    let(:geo_node_attributes) { { url: 'http://example.com', geo_node_key_attributes: attributes_for(:key) } }
+    let(:geo_node) { create(:geo_node) }
+    let!(:original_fingerprint) { geo_node.geo_node_key.fingerprint }
+
+    def go
+      post :update, id: geo_node, geo_node: geo_node_attributes
+    end
+
+    context 'without add-on license' do
+      before do
+        allow(Gitlab::Geo).to receive(:license_allows?) { false }
+        go
+      end
+
+      it_behaves_like 'unlicensed geo action'
+    end
+
+    context 'with add-on license' do
+      before do
+        allow(Gitlab::Geo).to receive(:license_allows?).and_return(true)
+        go
+      end
+
+      it 'updates the node without changing the key' do
+        geo_node.reload
+
+        expect(geo_node.url.chomp('/')).to eq(geo_node_attributes[:url])
+        expect(geo_node.geo_node_key.fingerprint).to eq(original_fingerprint)
       end
     end
   end
 
   describe '#repair' do
     let(:geo_node) { create(:geo_node) }
-    subject { post :repair, id: geo_node }
+    def go
+      post :repair, id: geo_node
+    end
 
     before do
       allow(Gitlab::Geo).to receive(:license_allows?) { false }
-      subject
+      go
     end
 
     it_behaves_like 'unlicensed geo action'

spec/features/admin/admin_geo_nodes_spec.rb

+require 'spec_helper'
+
+RSpec.describe 'admin Geo Nodes', type: :feature do
+  let!(:geo_node) { create(:geo_node) }
+
+  before do
+    allow(Gitlab::Geo).to receive(:license_allows?).and_return(true)
+    sign_in(create(:admin))
+  end
+
+  it 'show all public Geo Nodes' do
+    visit admin_geo_nodes_path
+
+    page.within(find('.geo-nodes', match: :first)) do
+      expect(page).to have_content(geo_node.url)
+    end
+  end
+
+  describe 'create a new Geo Nodes' do
+    let(:new_ssh_key) { attributes_for(:key)[:key] }
+
+    before do
+      visit admin_geo_nodes_path
+    end
+
+    it 'creates a new Geo Node' do
+      check 'This is a primary node'
+      fill_in 'geo_node_url', with: 'https://test.gitlab.com'
+      fill_in 'geo_node_geo_node_key_attributes_key', with: new_ssh_key
+      click_button 'Add Node'
+
+      expect(current_path).to eq admin_geo_nodes_path
+
+      page.within(find('.geo-nodes', match: :first)) do
+        expect(page).to have_content(geo_node.url)
+      end
+    end
+  end
+
+  describe 'update an existing Geo Node' do
+    before do
+      visit admin_geo_nodes_path
+      page.within(find('.node-actions', match: :first)) do
+        page.click_link('Edit')
+      end
+    end
+
+    it 'updates an existing Geo Node' do
+      fill_in 'URL', with: 'http://newsite.com'
+      check 'This is a primary node'
+      click_button 'Save changes'
+
+      expect(current_path).to eq admin_geo_nodes_path
+
+      page.within(find('.geo-nodes', match: :first)) do
+        expect(page).to have_content('http://newsite.com')
+        expect(page).to have_content('Primary')
+      end
+    end
+  end
+
+  describe 'remove an existing Geo Node' do
+    before do
+      visit admin_geo_nodes_path
+    end
+
+    it 'removes an existing Geo Node' do
+      page.within(find('.node-actions', match: :first)) do
+        page.click_link('Remove')
+      end
+
+      expect(current_path).to eq admin_geo_nodes_path
+      expect(page).not_to have_css('.geo-nodes')
+    end
+  end
+end