Commit 65b9768ccfbdc3de682d66430601cf3af1b2a2f8
1 parent
edd2143d
Exists in
master
and in
4 other branches
Group ownership completely based on users_groups relation now
Before we have only owner_id to determine group owner With multiple owners per group we should get rid of owner_id in group. So from now @group.owner will always be nil but @group.owners return an actual array of users who can admin this group
Showing
12 changed files
with
32 additions
and
66 deletions
Show diff stats
app/controllers/admin/groups_controller.rb
... | ... | @@ -20,9 +20,9 @@ class Admin::GroupsController < Admin::ApplicationController |
20 | 20 | def create |
21 | 21 | @group = Group.new(params[:group]) |
22 | 22 | @group.path = @group.name.dup.parameterize if @group.name |
23 | - @group.owner = current_user | |
24 | 23 | |
25 | 24 | if @group.save |
25 | + @group.add_owner(current_user) | |
26 | 26 | redirect_to [:admin, @group], notice: 'Group was successfully created.' |
27 | 27 | else |
28 | 28 | render "new" |
... | ... | @@ -30,14 +30,7 @@ class Admin::GroupsController < Admin::ApplicationController |
30 | 30 | end |
31 | 31 | |
32 | 32 | def update |
33 | - group_params = params[:group].dup | |
34 | - owner_id =group_params.delete(:owner_id) | |
35 | - | |
36 | - if owner_id | |
37 | - @group.change_owner(User.find(owner_id)) | |
38 | - end | |
39 | - | |
40 | - if @group.update_attributes(group_params) | |
33 | + if @group.update_attributes(params[:group]) | |
41 | 34 | redirect_to [:admin, @group], notice: 'Group was successfully updated.' |
42 | 35 | else |
43 | 36 | render "edit" | ... | ... |
app/controllers/groups_controller.rb
... | ... | @@ -21,9 +21,9 @@ class GroupsController < ApplicationController |
21 | 21 | def create |
22 | 22 | @group = Group.new(params[:group]) |
23 | 23 | @group.path = @group.name.dup.parameterize if @group.name |
24 | - @group.owner = current_user | |
25 | 24 | |
26 | 25 | if @group.save |
26 | + @group.add_owner(current_user) | |
27 | 27 | redirect_to @group, notice: 'Group was successfully created.' |
28 | 28 | else |
29 | 29 | render action: "new" | ... | ... |
app/controllers/profiles/groups_controller.rb
... | ... | @@ -8,8 +8,8 @@ class Profiles::GroupsController < ApplicationController |
8 | 8 | def leave |
9 | 9 | @users_group = group.users_groups.where(user_id: current_user.id).first |
10 | 10 | |
11 | - if group.owner == current_user | |
12 | - redirect_to(profile_groups_path, alert: "You can't leave group. You must transfer it to another owner before leaving.") | |
11 | + if group.last_owner?(current_user) | |
12 | + redirect_to(profile_groups_path, alert: "You can't leave group. You must add at least one more owner to it.") | |
13 | 13 | else |
14 | 14 | @users_group.destroy |
15 | 15 | redirect_to(profile_groups_path, info: "You left #{group.name} group.") | ... | ... |
app/models/ability.rb
... | ... | @@ -79,7 +79,7 @@ class Ability |
79 | 79 | rules << project_admin_rules |
80 | 80 | end |
81 | 81 | |
82 | - if project.group && project.group.owners.include?(user) | |
82 | + if project.group && project.group.has_owner?(user) | |
83 | 83 | rules << project_admin_rules |
84 | 84 | end |
85 | 85 | |
... | ... | @@ -159,7 +159,7 @@ class Ability |
159 | 159 | end |
160 | 160 | |
161 | 161 | # Only group owner and administrators can manage group |
162 | - if group.owners.include?(user) || user.admin? | |
162 | + if group.has_owner?(user) || user.admin? | |
163 | 163 | rules << [ |
164 | 164 | :manage_group, |
165 | 165 | :manage_namespace | ... | ... |
app/models/group.rb
... | ... | @@ -16,14 +16,12 @@ class Group < Namespace |
16 | 16 | has_many :users_groups, dependent: :destroy |
17 | 17 | has_many :users, through: :users_groups |
18 | 18 | |
19 | - after_create :add_owner | |
20 | - | |
21 | 19 | def human_name |
22 | 20 | name |
23 | 21 | end |
24 | 22 | |
25 | 23 | def owners |
26 | - @owners ||= (users_groups.owners.map(&:user) << owner).uniq | |
24 | + @owners ||= users_groups.owners.map(&:user) | |
27 | 25 | end |
28 | 26 | |
29 | 27 | def add_users(user_ids, group_access) |
... | ... | @@ -36,20 +34,19 @@ class Group < Namespace |
36 | 34 | self.users_groups.create(user_id: user.id, group_access: group_access) |
37 | 35 | end |
38 | 36 | |
39 | - def change_owner(user) | |
40 | - self.owner = user | |
41 | - membership = users_groups.where(user_id: user.id).first | |
37 | + def add_owner(user) | |
38 | + self.add_user(user, UsersGroup::OWNER) | |
39 | + end | |
42 | 40 | |
43 | - if membership | |
44 | - membership.update_attributes(group_access: UsersGroup::OWNER) | |
45 | - else | |
46 | - add_owner | |
47 | - end | |
41 | + def has_owner?(user) | |
42 | + owners.include?(user) | |
48 | 43 | end |
49 | 44 | |
50 | - private | |
45 | + def last_owner?(user) | |
46 | + has_owner?(user) && owners.size == 1 | |
47 | + end | |
51 | 48 | |
52 | - def add_owner | |
53 | - self.add_users([owner.id], UsersGroup::OWNER) | |
49 | + def members | |
50 | + users_groups | |
54 | 51 | end |
55 | 52 | end | ... | ... |
app/models/namespace.rb
... | ... | @@ -20,7 +20,7 @@ class Namespace < ActiveRecord::Base |
20 | 20 | has_many :projects, dependent: :destroy |
21 | 21 | belongs_to :owner, class_name: "User" |
22 | 22 | |
23 | - validates :owner, presence: true | |
23 | + validates :owner, presence: true, unless: ->(n) { n.type == "Group" } | |
24 | 24 | validates :name, presence: true, uniqueness: true, |
25 | 25 | length: { within: 0..255 }, |
26 | 26 | format: { with: Gitlab::Regex.name_regex, | ... | ... |
app/models/project.rb
... | ... | @@ -249,10 +249,10 @@ class Project < ActiveRecord::Base |
249 | 249 | end |
250 | 250 | |
251 | 251 | def owner |
252 | - if namespace | |
253 | - namespace_owner | |
252 | + if group | |
253 | + group | |
254 | 254 | else |
255 | - creator | |
255 | + namespace.try(:owner) | |
256 | 256 | end |
257 | 257 | end |
258 | 258 | |
... | ... | @@ -276,10 +276,6 @@ class Project < ActiveRecord::Base |
276 | 276 | end |
277 | 277 | end |
278 | 278 | |
279 | - def namespace_owner | |
280 | - namespace.try(:owner) | |
281 | - end | |
282 | - | |
283 | 279 | def path_with_namespace |
284 | 280 | if namespace |
285 | 281 | namespace.path + '/' + path | ... | ... |
app/models/user.rb
... | ... | @@ -135,7 +135,7 @@ class User < ActiveRecord::Base |
135 | 135 | # Remove user from all groups |
136 | 136 | user.users_groups.find_each do |membership| |
137 | 137 | # skip owned resources |
138 | - next if membership.group.owners.include?(user) | |
138 | + next if membership.group.last_owner?(user) | |
139 | 139 | |
140 | 140 | return false unless membership.destroy |
141 | 141 | end | ... | ... |
app/services/system_hooks_service.rb
... | ... | @@ -23,13 +23,15 @@ class SystemHooksService |
23 | 23 | |
24 | 24 | case model |
25 | 25 | when Project |
26 | + owner = model.owner | |
27 | + | |
26 | 28 | data.merge!({ |
27 | 29 | name: model.name, |
28 | 30 | path: model.path, |
29 | 31 | path_with_namespace: model.path_with_namespace, |
30 | 32 | project_id: model.id, |
31 | - owner_name: model.owner.name, | |
32 | - owner_email: model.owner.email | |
33 | + owner_name: owner.name, | |
34 | + owner_email: owner.respond_to?(:email) ? owner.email : nil | |
33 | 35 | }) |
34 | 36 | when User |
35 | 37 | data.merge!({ | ... | ... |
app/views/admin/groups/index.html.haml
... | ... | @@ -31,11 +31,8 @@ |
31 | 31 | |
32 | 32 | .clearfix.light.append-bottom-10 |
33 | 33 | %span |
34 | - %b Owner: | |
35 | - - if group.owner | |
36 | - = link_to group.owner_name, admin_user_path(group.owner) | |
37 | - - else | |
38 | - (deleted) | |
34 | + %b Members: | |
35 | + %span.badge= group.members.size | |
39 | 36 | \| |
40 | 37 | %span |
41 | 38 | %b Projects: | ... | ... |
app/views/admin/groups/show.html.haml
... | ... | @@ -25,26 +25,6 @@ |
25 | 25 | = @group.description |
26 | 26 | |
27 | 27 | %li |
28 | - %span.light Owned by: | |
29 | - %strong | |
30 | - - if @group.owner | |
31 | - = link_to @group.owner_name, admin_user_path(@group.owner) | |
32 | - - else | |
33 | - (deleted) | |
34 | - .pull-right | |
35 | - = link_to "#", class: "btn btn-small change-owner-link" do | |
36 | - %i.icon-edit | |
37 | - Change owner | |
38 | - %li.change-owner-holder.hide.bgred | |
39 | - .form-holder | |
40 | - %strong.cred New Owner: | |
41 | - = form_for [:admin, @group] do |f| | |
42 | - = users_select_tag(:"group[owner_id]") | |
43 | - .prepend-top-10 | |
44 | - = f.submit 'Change Owner', class: "btn btn-remove" | |
45 | - = link_to "Cancel", "#", class: "btn change-owner-cancel-link" | |
46 | - | |
47 | - %li | |
48 | 28 | %span.light Created at: |
49 | 29 | %strong |
50 | 30 | = @group.created_at.stamp("March 1, 1999") |
... | ... | @@ -92,4 +72,5 @@ |
92 | 72 | = link_to user.name, admin_user_path(user) |
93 | 73 | %span.pull-right.light |
94 | 74 | = member.human_access |
95 | - | |
75 | + = link_to group_users_group_path(@group, member), confirm: remove_user_from_group_message(@group, user), method: :delete, remote: true, class: "btn-tiny btn btn-remove", title: 'Remove user from group' do | |
76 | + %i.icon-minus.icon-white | ... | ... |
app/views/admin/projects/show.html.haml