UI and copywriting improvements13948-access-request-to-projects-and-groups

+ Move 'Edit Project/Group' out of membership-related partial
+ Show the access request buttons only to logged-in users
+ Put the request access buttons out of in a more visible button
+ Improve the copy in the #remove_member_message helper

Signed-off-by: Rémy Coutable <remy@rymai.me>

8 files changed, 41 insertions, 48 deletions

diff --git a/app/models/ability.rb b/app/models/ability.rb
index 90156bf130c..647a73aa1ce 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -460,8 +460,6 @@ class Ability
           rules << :destroy_group_member
         elsif user == target_user
           rules << :destroy_group_member
-        elsif subject.request? && user == subject.created_by
-          rules << :destroy_group_member
         end
       end
 
@@ -481,8 +479,6 @@ class Ability
           rules << :destroy_project_member
         elsif user == target_user
           rules << :destroy_project_member
-        elsif subject.request? && user == subject.created_by
-          rules << :destroy_project_member
         end
       end
 
diff --git a/app/models/concerns/access_requestable.rb b/app/models/concerns/access_requestable.rb
index cf37284e31a..eedd32a729f 100644
--- a/app/models/concerns/access_requestable.rb
+++ b/app/models/concerns/access_requestable.rb
@@ -10,18 +10,7 @@ module AccessRequestable
   def request_access(user)
     members.create(
       access_level: Gitlab::Access::DEVELOPER,
-      created_by: user,
+      user: user,
       requested_at: Time.now.utc)
   end
-
-  def access_requested?(user)
-    members.where(created_by_id: user.id).where.not(requested_at: nil).any?
-  end
-
-  private
-
-  # Returns a `<entities>_members` association, e.g.: project_members, group_members
-  def members
-    @members ||= send("#{self.class.to_s.underscore}_members".to_sym)
-  end
 end
diff --git a/app/models/group.rb b/app/models/group.rb
index 520cbd0283c..b8dffe9f5b9 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -8,7 +8,7 @@ class Group < Namespace
 
   has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember'
   alias_method :members, :group_members
-  has_many :users, through: :group_members
+  has_many :users, -> { where(members: { requested_at: nil }) }, through: :group_members
   has_many :project_group_links, dependent: :destroy
   has_many :shared_projects, through: :project_group_links, source: :project
   has_many :notification_settings, dependent: :destroy, as: :source
diff --git a/app/models/member.rb b/app/models/member.rb
index 5c3a5eab406..cea6d259760 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -8,7 +8,7 @@ class Member < ActiveRecord::Base
   belongs_to :user
   belongs_to :source, polymorphic: true
 
-  validates :user, presence: true, unless: :pending?
+  validates :user, presence: true, unless: :invite?
   validates :source, presence: true
   validates :user_id, uniqueness: { scope: [:source_type, :source_id],
                                     message: "already exists in source",
@@ -27,16 +27,17 @@ class Member < ActiveRecord::Base
     }
 
   scope :invite, -> { where.not(invite_token: nil) }
+  scope :non_invite, -> { where(invite_token: nil) }
   scope :request, -> { where.not(requested_at: nil) }
   scope :non_request, -> { where(requested_at: nil) }
-  scope :non_pending, -> { where.not(user_id: nil) }
+  scope :non_pending, -> { non_request.non_invite }
 
   scope :guests, -> { where(access_level: GUEST) }
   scope :reporters, -> { where(access_level: REPORTER) }
   scope :developers, -> { where(access_level: DEVELOPER) }
   scope :masters,  -> { where(access_level: MASTER) }
   scope :owners,  -> { where(access_level: OWNER) }
-  scope :admins,  -> { where(access_level: [OWNER, MASTER]) }
+  scope :owners_and_masters,  -> { where(access_level: [OWNER, MASTER]) }
 
   before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? }
 
@@ -46,6 +47,7 @@ class Member < ActiveRecord::Base
   after_create :post_create_hook, unless: :pending?
   after_update :post_update_hook, unless: :pending?
   after_destroy :post_destroy_hook, unless: :pending?
+  after_destroy :post_decline_request, if: :request?
 
   delegate :name, :username, :email, to: :user, prefix: true
 
@@ -102,36 +104,31 @@ class Member < ActiveRecord::Base
     end
   end
 
-  def pending?
-    request? || invite?
+  def real_source_type
+    source_type
+  end
+
+  def invite?
+    self.invite_token.present?
   end
 
   def request?
-    user.nil? && created_by.present? && requested_at.present?
+    requested_at.present?
   end
 
-  def invite?
-    self.invite_token.present?
+  def pending?
+    invite? || request?
   end
 
   def accept_request
     return false unless request?
 
-    updated = self.update(user: created_by, requested_at: nil)
+    updated = self.update(requested_at: nil)
     after_accept_request if updated
 
     updated
   end
 
-  def decline_request
-    return false unless request?
-
-    self.destroy
-    after_decline_request if destroyed?
-
-    destroyed?
-  end
-
   def accept_invite!(new_user)
     return false unless invite?
 
@@ -217,7 +214,7 @@ class Member < ActiveRecord::Base
     post_create_hook
   end
 
-  def after_decline_request
+  def post_decline_request
     # override in subclass
   end
 
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 476b4816b90..363db877968 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -20,6 +20,11 @@ class GroupMember < Member
     access_level
   end
 
+  # Because source_type is `Namespace`...
+  def real_source_type
+    'Group'
+  end
+
   private
 
   def send_invite
@@ -60,8 +65,8 @@ class GroupMember < Member
     super
   end
 
-  def after_decline_request
-    notification_service.decline_group_access_request(group, created_by)
+  def post_decline_request
+    notification_service.decline_group_access_request(self)
 
     super
   end
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index c6fd1a5c3d1..250ee04fd1d 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -152,8 +152,8 @@ class ProjectMember < Member
     super
   end
 
-  def after_decline_request
-    notification_service.decline_project_access_request(project, created_by)
+  def post_decline_request
+    notification_service.decline_project_access_request(self)
 
     super
   end
diff --git a/app/models/project.rb b/app/models/project.rb
index ef665373495..0d2e612436a 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -104,7 +104,8 @@ class Project < ActiveRecord::Base
   has_many :hooks,              dependent: :destroy, class_name: 'ProjectHook'
   has_many :protected_branches, dependent: :destroy
   has_many :project_members,    dependent: :destroy, as: :source, class_name: 'ProjectMember'
-  has_many :users, through: :project_members
+  alias_method :members, :project_members
+  has_many :users, -> { where(members: { requested_at: nil }) }, through: :project_members
   has_many :deploy_keys_projects, dependent: :destroy
   has_many :deploy_keys, through: :deploy_keys_projects
   has_many :users_star_projects, dependent: :destroy
@@ -690,6 +691,7 @@ class Project < ActiveRecord::Base
                                end
                              end
   end
+  alias_method :human_name, :name_with_namespace
 
   def path_with_namespace
     if namespace
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 7fb17df0e96..73e736820af 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -22,12 +22,12 @@ class ProjectTeam
   end
 
   def find_member(user_id)
-    member = project.project_members.find_by(user_id: user_id)
+    member = project.members.non_request.find_by(user_id: user_id)
 
     # If user is not in project members
     # we should check for group membership
     if group && !member
-      member = group.group_members.find_by(user_id: user_id)
+      member = group.members.non_request.find_by(user_id: user_id)
     end
 
     member
@@ -128,12 +128,16 @@ class ProjectTeam
     end
   end
 
+  def human_max_access(user_id)
+    Gitlab::Access.options_with_owner.key(max_member_access(user_id))
+  end
+
   # This method assumes project and group members are eager loaded for optimal
   # performance.
   def max_member_access(user_id)
     access = []
 
-    project.project_members.each do |member|
+    project.members.non_request.each do |member|
       if member.user_id == user_id
         access << member.access_field if member.access_field
         break
@@ -141,7 +145,7 @@ class ProjectTeam
     end
 
     if group
-      group.group_members.each do |member|
+      group.members.non_request.each do |member|
         if member.user_id == user_id
           access << member.access_field if member.access_field
           break
@@ -174,14 +178,14 @@ class ProjectTeam
   end
 
   def fetch_members(level = nil)
-    project_members = project.project_members
-    group_members = group ? group.group_members : []
+    project_members = project.members.non_request
+    group_members = group ? group.members.non_request : []
     invited_members = []
 
     if project.invited_groups.any? && project.allowed_to_share_with_group?
       project.project_group_links.each do |group_link|
         invited_group = group_link.group
-        im = invited_group.group_members
+        im = invited_group.members.non_request
 
         if level
           int_level = GroupMember.access_level_roles[level.to_s.singularize.titleize]