Get rid of skipping callbacks in production code. Dont trigger gitolite more than once on import in group

7 files changed, 72 insertions, 56 deletions

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 271647c783c..47143624ec4 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -99,11 +99,10 @@ class ProjectsController < ProjectResourceController
   def destroy
     return access_denied! unless can?(current_user, :remove_project, project)
 
-    # Disable the UsersProject update_repository call, otherwise it will be
-    # called once for every person removed from the project
-    UsersProject.skip_callback(:destroy, :after, :update_repository)
+    # Delete team first in order to prevent multiple gitolite calls
+    project.truncate_team
+
     project.destroy
-    UsersProject.set_callback(:destroy, :after, :update_repository)
 
     respond_to do |format|
       format.html { redirect_to root_path }
diff --git a/app/models/group.rb b/app/models/group.rb
index 5022fcf48f6..f16c70a6864 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -13,9 +13,11 @@
 
 class Group < Namespace
   def add_users_to_project_teams(user_ids, project_access)
-    projects.each do |project|
-      project.add_users_ids_to_team(user_ids, project_access)
-    end
+    UsersProject.add_users_into_projects(
+      projects.map(&:id),
+      user_ids,
+      project_access
+    )
   end
 
   def users
diff --git a/app/models/users_project.rb b/app/models/users_project.rb
index 34377aa5e66..23b83f5203d 100644
--- a/app/models/users_project.rb
+++ b/app/models/users_project.rb
@@ -23,11 +23,13 @@ class UsersProject < ActiveRecord::Base
   belongs_to :user
   belongs_to :project
 
-  after_save :update_repository
-  after_destroy :update_repository
+  attr_accessor :skip_git
+
+  after_save :update_repository, unless: :skip_git?
+  after_destroy :update_repository, unless: :skip_git?
 
   validates :user, presence: true
-  validates :user_id, uniqueness: { :scope => [:project_id], message: "already exists in project" }
+  validates :user_id, uniqueness: { scope: [:project_id], message: "already exists in project" }
   validates :project_access, inclusion: { in: [GUEST, REPORTER, DEVELOPER, MASTER] }, presence: true
   validates :project, presence: true
 
@@ -36,76 +38,84 @@ class UsersProject < ActiveRecord::Base
   scope :in_project, ->(project) { where(project_id: project.id) }
 
   class << self
-    def import_team(source_project, target_project)
-      UsersProject.without_repository_callback do
-        UsersProject.transaction do
-          team = source_project.users_projects.all
-
-          team.each do |tm|
-            # Skip if user already present in team
-            next if target_project.users.include?(tm.user)
-
-            new_tm = tm.dup
-            new_tm.id = nil
-            new_tm.project_id = target_project.id
-            new_tm.save
+    def add_users_into_projects(project_ids, user_ids, project_access)
+      UsersProject.transaction do
+        project_ids.each do |project_id|
+          user_ids.each do |user_id|
+            users_project = UsersProject.new(project_access: project_access, user_id: user_id)
+            users_project.project_id = project_id
+            users_project.skip_git = true
+            users_project.save
           end
         end
+        Gitlab::Gitolite.new.update_repositories(Project.where(id: project_ids))
       end
 
-      target_project.update_repository
       true
     rescue
       false
     end
 
-    def without_repository_callback
-      UsersProject.skip_callback(:destroy, :after, :update_repository)
-      yield
-      UsersProject.set_callback(:destroy, :after, :update_repository)
+    def import_team(source_project, target_project)
+      source_team = source_project.users_projects.all
+      target_team = target_project.users_projects.all
+      target_user_ids = target_team.map(&:user_id)
+
+      source_team.reject! do |tm|
+        # Skip if user already present in team
+        target_user_ids.include?(tm.user_id)
+      end
+
+      source_team.map! do |tm|
+        new_tm = tm.dup
+        new_tm.id = nil
+        new_tm.project_id = target_project.id
+        new_tm.skip_git = true
+        new_tm
+      end
+
+      UsersProject.transaction do
+        source_team.each do |tm|
+          tm.save
+        end
+        target_project.update_repository
+      end
+
+      true
+    rescue
+      false
     end
 
     def bulk_delete(project, user_ids)
       UsersProject.transaction do
-        UsersProject.where(:user_id => user_ids, :project_id => project.id).each do |users_project|
+        UsersProject.where(user_id: user_ids, project_id: project.id).each do |users_project|
+          users_project.skip_git = true
           users_project.destroy
         end
+
+        project.update_repository
       end
     end
 
     def bulk_update(project, user_ids, project_access)
       UsersProject.transaction do
-        UsersProject.where(:user_id => user_ids, :project_id => project.id).each do |users_project|
+        UsersProject.where(user_id: user_ids, project_id: project.id).each do |users_project|
           users_project.project_access = project_access
+          users_project.skip_git = true
           users_project.save
         end
+        project.update_repository
       end
     end
 
+    # TODO: depreceate in future in favor of add_users_into_projects
     def bulk_import(project, user_ids, project_access)
-      UsersProject.transaction do
-        user_ids.each do |user_id|
-          users_project = UsersProject.new(
-            project_access: project_access,
-            user_id: user_id
-          )
-          users_project.project = project
-          users_project.save
-        end
-      end
+      add_users_into_projects([project.id], user_ids, project_access)
     end
 
+    # TODO: depreceate in future in favor of add_users_into_projects
     def user_bulk_import(user, project_ids, project_access)
-      UsersProject.transaction do
-        project_ids.each do |project_id|
-          users_project = UsersProject.new(
-            project_access: project_access,
-          )
-          users_project.project_id = project_id
-          users_project.user_id = user.id
-          users_project.save
-        end
-      end
+      add_users_into_projects(project_ids, [user.id], project_access)
     end
 
     def access_roles
@@ -133,4 +143,8 @@ class UsersProject < ActiveRecord::Base
   def repo_access_human
     self.class.access_roles.invert[self.project_access]
   end
+
+  def skip_git?
+    !!@skip_git
+  end
 end
diff --git a/app/roles/team.rb b/app/roles/team.rb
index a7ba0588cf5..0e431244590 100644
--- a/app/roles/team.rb
+++ b/app/roles/team.rb
@@ -34,19 +34,20 @@ module Team
   # with same access role by user ids
   def add_users_ids_to_team(users_ids, access_role)
     UsersProject.bulk_import(self, users_ids, access_role)
-    self.update_repository
   end
 
   # Update multiple project users
   # to same access role by user ids
   def update_users_ids_to_role(users_ids, access_role)
     UsersProject.bulk_update(self, users_ids, access_role)
-    self.update_repository
   end
 
   # Delete multiple users from project by user ids
   def delete_users_ids_from_team(users_ids)
     UsersProject.bulk_delete(self, users_ids)
-    self.update_repository
+  end
+
+  def truncate_team
+    UsersProject.bulk_delete(self, self.users.map(&:id))
   end
 end
diff --git a/app/views/groups/_new_group_member.html.haml b/app/views/groups/_new_group_member.html.haml
index 75023057512..ddda038a6c3 100644
--- a/app/views/groups/_new_group_member.html.haml
+++ b/app/views/groups/_new_group_member.html.haml
@@ -5,7 +5,7 @@
     %h6 1. Choose people you want in the team
     .clearfix
       = f.label :user_ids, "People"
-      .input= select_tag(:user_ids, options_from_collection_for_select(User.active, :id, :name), {data: {placeholder: "Select users"}, class: "chosen xxlarge", multiple: true})
+      .input= select_tag(:user_ids, options_from_collection_for_select(User.active.order('name ASC'), :id, :name), {data: {placeholder: "Select users"}, class: "chosen xxlarge", multiple: true})
 
     %h6 2. Set access level for them
     .clearfix
diff --git a/app/views/groups/_new_member.html.haml b/app/views/groups/_new_member.html.haml
index f48c2c23d83..c36d1920caf 100644
--- a/app/views/groups/_new_member.html.haml
+++ b/app/views/groups/_new_member.html.haml
@@ -5,7 +5,7 @@
     %h6 1. Choose people you want in the team
     .clearfix
       = f.label :user_ids, "People"
-      .input= select_tag(:user_ids, options_from_collection_for_select(User.not_in_project(@project).all, :id, :name), {data: {placeholder: "Select users"}, class: "chosen xxlarge", multiple: true})
+      .input= select_tag(:user_ids, options_from_collection_for_select(User.not_in_project(@project).order('name').all, :id, :name), {data: {placeholder: "Select users"}, class: "chosen xxlarge", multiple: true})
 
     %h6 2. Set access level for them
     .clearfix
diff --git a/app/views/team_members/import.html.haml b/app/views/team_members/import.html.haml
index 34f7fb03288..de82f416248 100644
--- a/app/views/team_members/import.html.haml
+++ b/app/views/team_members/import.html.haml
@@ -9,7 +9,7 @@
   %p.slead Choose project you want to use as team source:
   .padded
     = label_tag :source_project_id, "Project"
-    .input= select_tag(:source_project_id, options_from_collection_for_select(current_user.projects, :id, :name), prompt: "Select project", class: "chosen xxlarge", required: true)
+    .input= select_tag(:source_project_id, options_from_collection_for_select(current_user.authorized_projects, :id, :name_with_namespace), prompt: "Select project", class: "chosen xxlarge", required: true)
 
   .actions
     = submit_tag 'Import', class: "btn save-btn"