Get rid of skipping callbacks in production code. Dont trigger gitolite more tha…

…n once on import in group

Get rid of skipping callbacks in production code. Dont trigger gitolite more tha…
…n once on import in group
Dmitriy Zaporozhets
1 parent 151ada76
Showing 7 changed files with 72 additions and 56 deletions Show diff stats
app/controllers/projects_controller.rb
app/models/group.rb
app/models/users_project.rb
app/roles/team.rb
app/views/groups/_new_group_member.html.haml
app/views/groups/_new_member.html.haml
app/views/team_members/import.html.haml
@@ -99,11 +99,10 @@ class ProjectsController &lt; 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 }
@@ -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
@@ -23,11 +23,13 @@ class UsersProject &lt; 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 &lt; 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 &lt; ActiveRecord::Base
   def repo_access_human
     self.class.access_roles.invert[self.project_access]
   end
+
+  def skip_git?
+    !!@skip_git
+  end
 end
@@ -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
@@ -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
@@ -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
@@ -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"
...	...	@@ -99,11 +99,10 @@ class ProjectsController < ProjectResourceController
99	99	def destroy
100	100	return access_denied! unless can?(current_user, :remove_project, project)
101	101
102		- # Disable the UsersProject update_repository call, otherwise it will be
103		- # called once for every person removed from the project
104		- UsersProject.skip_callback(:destroy, :after, :update_repository)
	102	+ # Delete team first in order to prevent multiple gitolite calls
	103	+ project.truncate_team
	104	+
105	105	project.destroy
106		- UsersProject.set_callback(:destroy, :after, :update_repository)
107	106
108	107	respond_to do \|format\|
109	108	format.html { redirect_to root_path }
...	...
...	...	@@ -13,9 +13,11 @@
13	13
14	14	class Group < Namespace
15	15	def add_users_to_project_teams(user_ids, project_access)
16		- projects.each do \|project\|
17		- project.add_users_ids_to_team(user_ids, project_access)
18		- end
	16	+ UsersProject.add_users_into_projects(
	17	+ projects.map(&:id),
	18	+ user_ids,
	19	+ project_access
	20	+ )
19	21	end
20	22
21	23	def users
...	...
...	...	@@ -23,11 +23,13 @@ class UsersProject < ActiveRecord::Base
23	23	belongs_to :user
24	24	belongs_to :project
25	25
26		- after_save :update_repository
27		- after_destroy :update_repository
	26	+ attr_accessor :skip_git
	27	+
	28	+ after_save :update_repository, unless: :skip_git?
	29	+ after_destroy :update_repository, unless: :skip_git?
28	30
29	31	validates :user, presence: true
30		- validates :user_id, uniqueness: { :scope => [:project_id], message: "already exists in project" }
	32	+ validates :user_id, uniqueness: { scope: [:project_id], message: "already exists in project" }
31	33	validates :project_access, inclusion: { in: [GUEST, REPORTER, DEVELOPER, MASTER] }, presence: true
32	34	validates :project, presence: true
33	35
...	...	@@ -36,76 +38,84 @@ class UsersProject < ActiveRecord::Base
36	38	scope :in_project, ->(project) { where(project_id: project.id) }
37	39
38	40	class << self
39		- def import_team(source_project, target_project)
40		- UsersProject.without_repository_callback do
41		- UsersProject.transaction do
42		- team = source_project.users_projects.all
43		-
44		- team.each do \|tm\|
45		- # Skip if user already present in team
46		- next if target_project.users.include?(tm.user)
47		-
48		- new_tm = tm.dup
49		- new_tm.id = nil
50		- new_tm.project_id = target_project.id
51		- new_tm.save
	41	+ def add_users_into_projects(project_ids, user_ids, project_access)
	42	+ UsersProject.transaction do
	43	+ project_ids.each do \|project_id\|
	44	+ user_ids.each do \|user_id\|
	45	+ users_project = UsersProject.new(project_access: project_access, user_id: user_id)
	46	+ users_project.project_id = project_id
	47	+ users_project.skip_git = true
	48	+ users_project.save
52	49	end
53	50	end
	51	+ Gitlab::Gitolite.new.update_repositories(Project.where(id: project_ids))
54	52	end
55	53
56		- target_project.update_repository
57	54	true
58	55	rescue
59	56	false
60	57	end
61	58
62		- def without_repository_callback
63		- UsersProject.skip_callback(:destroy, :after, :update_repository)
64		- yield
65		- UsersProject.set_callback(:destroy, :after, :update_repository)
	59	+ def import_team(source_project, target_project)
	60	+ source_team = source_project.users_projects.all
	61	+ target_team = target_project.users_projects.all
	62	+ target_user_ids = target_team.map(&:user_id)
	63	+
	64	+ source_team.reject! do \|tm\|
	65	+ # Skip if user already present in team
	66	+ target_user_ids.include?(tm.user_id)
	67	+ end
	68	+
	69	+ source_team.map! do \|tm\|
	70	+ new_tm = tm.dup
	71	+ new_tm.id = nil
	72	+ new_tm.project_id = target_project.id
	73	+ new_tm.skip_git = true
	74	+ new_tm
	75	+ end
	76	+
	77	+ UsersProject.transaction do
	78	+ source_team.each do \|tm\|
	79	+ tm.save
	80	+ end
	81	+ target_project.update_repository
	82	+ end
	83	+
	84	+ true
	85	+ rescue
	86	+ false
66	87	end
67	88
68	89	def bulk_delete(project, user_ids)
69	90	UsersProject.transaction do
70		- UsersProject.where(:user_id => user_ids, :project_id => project.id).each do \|users_project\|
	91	+ UsersProject.where(user_id: user_ids, project_id: project.id).each do \|users_project\|
	92	+ users_project.skip_git = true
71	93	users_project.destroy
72	94	end
	95	+
	96	+ project.update_repository
73	97	end
74	98	end
75	99
76	100	def bulk_update(project, user_ids, project_access)
77	101	UsersProject.transaction do
78		- UsersProject.where(:user_id => user_ids, :project_id => project.id).each do \|users_project\|
	102	+ UsersProject.where(user_id: user_ids, project_id: project.id).each do \|users_project\|
79	103	users_project.project_access = project_access
	104	+ users_project.skip_git = true
80	105	users_project.save
81	106	end
	107	+ project.update_repository
82	108	end
83	109	end
84	110
	111	+ # TODO: depreceate in future in favor of add_users_into_projects
85	112	def bulk_import(project, user_ids, project_access)
86		- UsersProject.transaction do
87		- user_ids.each do \|user_id\|
88		- users_project = UsersProject.new(
89		- project_access: project_access,
90		- user_id: user_id
91		- )
92		- users_project.project = project
93		- users_project.save
94		- end
95		- end
	113	+ add_users_into_projects([project.id], user_ids, project_access)
96	114	end
97	115
	116	+ # TODO: depreceate in future in favor of add_users_into_projects
98	117	def user_bulk_import(user, project_ids, project_access)
99		- UsersProject.transaction do
100		- project_ids.each do \|project_id\|
101		- users_project = UsersProject.new(
102		- project_access: project_access,
103		- )
104		- users_project.project_id = project_id
105		- users_project.user_id = user.id
106		- users_project.save
107		- end
108		- end
	118	+ add_users_into_projects(project_ids, [user.id], project_access)
109	119	end
110	120
111	121	def access_roles
...	...	@@ -133,4 +143,8 @@ class UsersProject < ActiveRecord::Base
133	143	def repo_access_human
134	144	self.class.access_roles.invert[self.project_access]
135	145	end
	146	+
	147	+ def skip_git?
	148	+ !!@skip_git
	149	+ end
136	150	end
...	...
...	...	@@ -34,19 +34,20 @@ module Team
34	34	# with same access role by user ids
35	35	def add_users_ids_to_team(users_ids, access_role)
36	36	UsersProject.bulk_import(self, users_ids, access_role)
37		- self.update_repository
38	37	end
39	38
40	39	# Update multiple project users
41	40	# to same access role by user ids
42	41	def update_users_ids_to_role(users_ids, access_role)
43	42	UsersProject.bulk_update(self, users_ids, access_role)
44		- self.update_repository
45	43	end
46	44
47	45	# Delete multiple users from project by user ids
48	46	def delete_users_ids_from_team(users_ids)
49	47	UsersProject.bulk_delete(self, users_ids)
50		- self.update_repository
	48	+ end
	49	+
	50	+ def truncate_team
	51	+ UsersProject.bulk_delete(self, self.users.map(&:id))
51	52	end
52	53	end
...	...
...	...	@@ -5,7 +5,7 @@
5	5	%h6 1. Choose people you want in the team
6	6	.clearfix
7	7	= f.label :user_ids, "People"
8		- .input= select_tag(:user_ids, options_from_collection_for_select(User.active, :id, :name), {data: {placeholder: "Select users"}, class: "chosen xxlarge", multiple: true})
	8	+ .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})
9	9
10	10	%h6 2. Set access level for them
11	11	.clearfix
...	...
...	...	@@ -9,7 +9,7 @@
9	9	%p.slead Choose project you want to use as team source:
10	10	.padded
11	11	= label_tag :source_project_id, "Project"
12		- .input= select_tag(:source_project_id, options_from_collection_for_select(current_user.projects, :id, :name), prompt: "Select project", class: "chosen xxlarge", required: true)
	12	+ .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)
13	13
14	14	.actions
15	15	= submit_tag 'Import', class: "btn save-btn"
...	...