From 785cbb79e255c8369ca5eb916207304f39d188ad Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 22 Jun 2017 08:55:07 +0200
Subject: refactor emails service

---
 app/controllers/admin/users_controller.rb     |  8 ++------
 app/controllers/profiles/emails_controller.rb |  4 +---
 app/models/user.rb                            |  2 --
 app/services/emails/destroy_service.rb        | 12 +++++++++++-
 lib/api/users.rb                              |  8 --------
 5 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 4c43d12d317..065ebf81793 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -136,7 +136,7 @@ class Admin::UsersController < Admin::ApplicationController
         # restore username to keep form action url.
         user.username = params[:id]
         format.html { render "edit" }
-        format.json { render json: result[:message], status: result[:status] }
+        format.json { render json: [result[:message]], status: result[:status] }
       end
     end
   end
@@ -152,11 +152,7 @@ class Admin::UsersController < Admin::ApplicationController
 
   def remove_email
     email = user.emails.find(params[:email_id])
-    Emails::DestroyService.new(current_user, self, email: email.email).execute
-
-    result = Users::UpdateService.new(current_user, @user).execute do |user|
-      user.update_secondary_emails!
-    end
+    Emails::DestroyService.new(current_user, user, email: email.email).execute
 
     respond_to do |format|
       if result[:status] == :success
diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index 40b43278439..5c7a5fa9a64 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -18,9 +18,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
 
   def destroy
     @email = current_user.emails.find(params[:id])
-    Emails::DestroyService.new(self, self, email: @email.email).execute
-
-    Users::UpdateService.new(current_user, current_user).execute { |user| user.update_secondary_emails! }
+    Emails::DestroyService.new(current_user, current_user, email: @email.email).execute
 
     respond_to do |format|
       format.html { redirect_to profile_emails_url, status: 302 }
diff --git a/app/models/user.rb b/app/models/user.rb
index 117f54cf312..d53837fbdb2 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -496,8 +496,6 @@ class User < ActiveRecord::Base
     if primary_email_record
       Emails::DestroyService.new(self, self, email: email).execute
       Emails::CreateService.new(self, self, email: email_was).execute
-
-      update_secondary_emails!
     end
   end
 
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index 1275d31efb0..8150918986c 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -3,7 +3,17 @@ module Emails
     def execute(skip_authorization: false)
       raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_manage_emails?
 
-      Email.find_by_email(@email).destroy
+      Email.find_by_email(@email).destroy && update_secondary_emails!
+    end
+
+    private
+
+    def update_secondary_emails!
+      result = ::Users::UpdateService.new(@current_user, @current_user).execute do |user|
+        user.update_secondary_emails!
+      end
+
+      result[:status] == 'success'
     end
   end
 end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 190e2e71884..07e7c774f2b 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -275,10 +275,6 @@ module API
         not_found!('Email') unless email
 
         Emails::DestroyService.new(current_user, user, email: email.email).execute(skip_authorization: true)
-
-        ::Users::UpdateService.new(current_user, user).execute do |user|
-          user.update_secondary_emails!
-        end
       end
 
       desc 'Delete a user. Available only for admins.' do
@@ -509,10 +505,6 @@ module API
         not_found!('Email') unless email
 
         Emails::DestroyService.new(current_user, current_user, email: email.email).execute
-
-        ::Users::UpdateService.new(current_user, current_user).execute  do |user|
-          user.update_secondary_emails!
-        end
       end
 
       desc 'Get a list of user activities'
-- 
cgit v1.2.1