Merge branch '35917_create_services_for_keys' into 'master'

created services for keys

See merge request gitlab-org/gitlab-ce!13331

18 files changed, 103 insertions, 49 deletions

diff --git a/app/controllers/admin/deploy_keys_controller.rb b/app/controllers/admin/deploy_keys_controller.rb
index e5cba774dcb..a7ab481519d 100644
--- a/app/controllers/admin/deploy_keys_controller.rb
+++ b/app/controllers/admin/deploy_keys_controller.rb
@@ -10,9 +10,8 @@ class Admin::DeployKeysController < Admin::ApplicationController
   end
 
   def create
-    @deploy_key = deploy_keys.new(create_params.merge(user: current_user))
-
-    if @deploy_key.save
+    @deploy_key = DeployKeys::CreateService.new(current_user, create_params.merge(public: true)).execute
+    if @deploy_key.persisted?
       redirect_to admin_deploy_keys_path
     else
       render 'new'
diff --git a/app/controllers/profiles/gpg_keys_controller.rb b/app/controllers/profiles/gpg_keys_controller.rb
index 6779cc6ddac..689c76059f6 100644
--- a/app/controllers/profiles/gpg_keys_controller.rb
+++ b/app/controllers/profiles/gpg_keys_controller.rb
@@ -7,9 +7,9 @@ class Profiles::GpgKeysController < Profiles::ApplicationController
   end
 
   def create
-    @gpg_key = current_user.gpg_keys.new(gpg_key_params)
+    @gpg_key = GpgKeys::CreateService.new(current_user, gpg_key_params).execute
 
-    if @gpg_key.save
+    if @gpg_key.persisted?
       redirect_to profile_gpg_keys_path
     else
       @gpg_keys = current_user.gpg_keys.select(&:persisted?)
diff --git a/app/controllers/profiles/keys_controller.rb b/app/controllers/profiles/keys_controller.rb
index f9f0e8eef83..89d6d7f1b52 100644
--- a/app/controllers/profiles/keys_controller.rb
+++ b/app/controllers/profiles/keys_controller.rb
@@ -11,9 +11,9 @@ class Profiles::KeysController < Profiles::ApplicationController
   end
 
   def create
-    @key = current_user.keys.new(key_params)
+    @key = Keys::CreateService.new(current_user, key_params).execute
 
-    if @key.save
+    if @key.persisted?
       redirect_to profile_key_path(@key)
     else
       @keys = current_user.keys.select(&:persisted?)
diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb
index c2e621fa190..cf8829ba95b 100644
--- a/app/controllers/projects/deploy_keys_controller.rb
+++ b/app/controllers/projects/deploy_keys_controller.rb
@@ -22,7 +22,7 @@ class Projects::DeployKeysController < Projects::ApplicationController
   end
 
   def create
-    @key = DeployKey.new(create_params.merge(user: current_user))
+    @key = DeployKeys::CreateService.new(current_user, create_params).execute
 
     unless @key.valid? && @project.deploy_keys << @key
       flash[:alert] = @key.errors.full_messages.join(', ').html_safe
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index 51768dd96bc..eae5eee4fee 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -28,10 +28,4 @@ class DeployKey < Key
   def can_push_to?(project)
     can_push? && has_access_to?(project)
   end
-
-  private
-
-  # we don't want to notify the user for deploy keys
-  def notify_user
-  end
 end
diff --git a/app/models/gpg_key.rb b/app/models/gpg_key.rb
index 1633acd4fa9..44deae4234b 100644
--- a/app/models/gpg_key.rb
+++ b/app/models/gpg_key.rb
@@ -36,7 +36,6 @@ class GpgKey < ActiveRecord::Base
 
   before_validation :extract_fingerprint, :extract_primary_keyid
   after_commit :update_invalid_gpg_signatures, on: :create
-  after_commit :notify_user, on: :create
 
   def primary_keyid
     super&.upcase
@@ -107,8 +106,4 @@ class GpgKey < ActiveRecord::Base
     # only allows one key
     self.primary_keyid = Gitlab::Gpg.primary_keyids_from_key(key).first
   end
-
-  def notify_user
-    NotificationService.new.new_gpg_key(self)
-  end
 end
diff --git a/app/models/key.rb b/app/models/key.rb
index a6b4dcfec0d..4fa6cac2fd0 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -28,7 +28,6 @@ class Key < ActiveRecord::Base
   delegate :name, :email, to: :user, prefix: true
 
   after_commit :add_to_shell, on: :create
-  after_commit :notify_user, on: :create
   after_create :post_create_hook
   after_commit :remove_from_shell, on: :destroy
   after_destroy :post_destroy_hook
@@ -118,8 +117,4 @@ class Key < ActiveRecord::Base
 
     "type is forbidden. Must be #{allowed_types}"
   end
-
-  def notify_user
-    NotificationService.new.new_key(self)
-  end
 end
diff --git a/app/services/deploy_keys/create_service.rb b/app/services/deploy_keys/create_service.rb
new file mode 100644
index 00000000000..16de3d08df2
--- /dev/null
+++ b/app/services/deploy_keys/create_service.rb
@@ -0,0 +1,7 @@
+module DeployKeys
+  class CreateService < Keys::BaseService
+    def execute
+      DeployKey.create(params.merge(user: user))
+    end
+  end
+end
diff --git a/app/services/gpg_keys/create_service.rb b/app/services/gpg_keys/create_service.rb
new file mode 100644
index 00000000000..e822a89c4d3
--- /dev/null
+++ b/app/services/gpg_keys/create_service.rb
@@ -0,0 +1,9 @@
+module GpgKeys
+  class CreateService < Keys::BaseService
+    def execute
+      key = user.gpg_keys.create(params)
+      notification_service.new_gpg_key(key) if key.persisted?
+      key
+    end
+  end
+end
diff --git a/app/services/keys/base_service.rb b/app/services/keys/base_service.rb
new file mode 100644
index 00000000000..545832d0bd4
--- /dev/null
+++ b/app/services/keys/base_service.rb
@@ -0,0 +1,13 @@
+module Keys
+  class BaseService
+    attr_accessor :user, :params
+
+    def initialize(user, params)
+      @user, @params = user, params
+    end
+
+    def notification_service
+      NotificationService.new
+    end
+  end
+end
diff --git a/app/services/keys/create_service.rb b/app/services/keys/create_service.rb
new file mode 100644
index 00000000000..e2e5a6c46c5
--- /dev/null
+++ b/app/services/keys/create_service.rb
@@ -0,0 +1,9 @@
+module Keys
+  class CreateService < ::Keys::BaseService
+    def execute
+      key = user.keys.create(params)
+      notification_service.new_key(key) if key.persisted?
+      key
+    end
+  end
+end
diff --git a/changelogs/unreleased/35917_create_services_for_keys.yml b/changelogs/unreleased/35917_create_services_for_keys.yml
new file mode 100644
index 00000000000..e7cad5a11d5
--- /dev/null
+++ b/changelogs/unreleased/35917_create_services_for_keys.yml
@@ -0,0 +1,4 @@
+---
+title: creation of keys moved to services
+merge_request: 13331
+author: haseebeqx
diff --git a/spec/models/gpg_key_spec.rb b/spec/models/gpg_key_spec.rb
index 9c99c3e5c08..fadc8bfeb61 100644
--- a/spec/models/gpg_key_spec.rb
+++ b/spec/models/gpg_key_spec.rb
@@ -140,18 +140,6 @@ describe GpgKey do
     end
   end
 
-  describe 'notification', :mailer do
-    let(:user) { create(:user) }
-
-    it 'sends a notification' do
-      perform_enqueued_jobs do
-        create(:gpg_key, user: user)
-      end
-
-      should_email(user)
-    end
-  end
-
   describe '#revoke' do
     it 'invalidates all associated gpg signatures and destroys the key' do
       gpg_key = create :gpg_key
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index 96baeaff0a4..dbc4aba8547 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -169,16 +169,4 @@ describe Key, :mailer do
       expect(described_class.new(key: " #{valid_key} ").key).to eq(valid_key)
     end
   end
-
-  describe 'notification' do
-    let(:user) { create(:user) }
-
-    it 'sends a notification' do
-      perform_enqueued_jobs do
-        create(:key, user: user)
-      end
-
-      should_email(user)
-    end
-  end
 end
diff --git a/spec/services/deploy_keys/create_service_spec.rb b/spec/services/deploy_keys/create_service_spec.rb
new file mode 100644
index 00000000000..7a604c0cadd
--- /dev/null
+++ b/spec/services/deploy_keys/create_service_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe DeployKeys::CreateService do
+  let(:user) { create(:user) }
+  let(:params) { attributes_for(:deploy_key) }
+
+  subject { described_class.new(user, params) }
+
+  it "creates a deploy key" do
+    expect { subject.execute }.to change { DeployKey.where(params.merge(user: user)).count }.by(1)
+  end
+end
diff --git a/spec/services/gpg_keys/create_service_spec.rb b/spec/services/gpg_keys/create_service_spec.rb
new file mode 100644
index 00000000000..20382a3a618
--- /dev/null
+++ b/spec/services/gpg_keys/create_service_spec.rb
@@ -0,0 +1,21 @@
+require 'spec_helper'
+
+describe GpgKeys::CreateService do
+  let(:user) { create(:user) }
+  let(:params) { attributes_for(:gpg_key) }
+
+  subject { described_class.new(user, params) }
+
+  context 'notification', :mailer do
+    it 'sends a notification' do
+      perform_enqueued_jobs do
+        subject.execute
+      end
+      should_email(user)
+    end
+  end
+
+  it 'creates a gpg key' do
+    expect { subject.execute }.to change { user.gpg_keys.where(params).count }.by(1)
+  end
+end
diff --git a/spec/services/keys/create_service_spec.rb b/spec/services/keys/create_service_spec.rb
new file mode 100644
index 00000000000..bcb436c1e46
--- /dev/null
+++ b/spec/services/keys/create_service_spec.rb
@@ -0,0 +1,21 @@
+require 'spec_helper'
+
+describe Keys::CreateService do
+  let(:user) { create(:user) }
+  let(:params) { attributes_for(:key) }
+
+  subject { described_class.new(user, params) }
+
+  context 'notification', :mailer do
+    it 'sends a notification' do
+      perform_enqueued_jobs do
+        subject.execute
+      end
+      should_email(user)
+    end
+  end
+
+  it 'creates a key' do
+    expect { subject.execute }.to change { user.keys.where(params).count }.by(1)
+  end
+end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 3e493148b32..f4b36eb7eeb 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -84,7 +84,6 @@ describe NotificationService, :mailer do
       let!(:key) { create(:personal_key, key_options) }
 
       it { expect(notification.new_key(key)).to be_truthy }
-      it { should_email(key.user) }
 
       describe 'never emails the ghost user' do
         let(:key_options) { { user: User.ghost } }