Merge branch '27742-display-member-role-per-project' into 'master'

Resolve "Display member role per project"

Closes #27742 and #40340

See merge request gitlab-org/gitlab-ce!15387

app/assets/javascripts/groups/components/group_item.vue

@@ -123,18 +123,23 @@ export default {
           :title="group.fullName"
           class="no-expand"
           data-placement="top"
-        >
-          {{group.name}}
-        </a>
+        >{{
+          // ending bracket must be by closing tag to prevent
+          // link hover text-decoration from over-extending
+          group.name
+        }}</a>
         <span
           v-if="group.permission"
-          class="access-type"
+          class="user-access-role"
         >
-          {{s__('GroupsTreeRole|as')}} {{group.permission}}
+          {{group.permission}}
         </span>
       </div>
       <div
-        class="description">{{group.description}}</div>
+        v-if="group.description"
+        class="description">
+        {{group.description}}
+      </div>
     </div>
     <group-folder
       v-if="group.isOpen && hasChildren"

app/assets/javascripts/notes/components/note_actions.vue

@@ -86,7 +86,7 @@
   <div class="note-actions">
     <span
       v-if="accessLevel"
-      class="note-role note-role-access">{{accessLevel}}</span>
+      class="note-role user-access-role">{{accessLevel}}</span>
     <div
       v-if="canAddAwardEmoji"
       class="note-actions-item">

app/assets/stylesheets/framework/lists.scss

@@ -457,13 +457,11 @@ ul.indent-list {
 
 ul.group-list-tree {
   li.group-row {
-    &.has-description {
-      .title {
-        line-height: inherit;
-      }
+    &.has-description .title {
+      line-height: inherit;
     }
 
-    .title {
+    &:not(.has-description) .title {
       line-height: $list-text-height;
     }
   }

app/assets/stylesheets/framework/variables.scss

@@ -408,7 +408,6 @@ $location-icon-color: #e7e9ed;
  *  Notes
  */
 $notes-light-color: $gl-text-color-secondary;
-$notes-role-color: $gl-text-color-secondary;
 $note-disabled-comment-color: #b2b2b2;
 $note-targe3-outside: #fffff0;
 $note-targe3-inside: #ffffd3;
@@ -557,6 +556,7 @@ $jq-ui-default-color: #777;
 /*
 * Label
 */
+$label-padding: 7px;
 $label-gray-bg: #f8fafc;
 $label-inverse-bg: #333;
 $label-remove-border: rgba(0, 0, 0, .1);

app/assets/stylesheets/pages/groups.scss

@@ -212,3 +212,15 @@
     height: 50px;
   }
 }
+
+.user-access-role {
+  display: inline-block;
+  color: $gl-text-color-secondary;
+  font-size: 12px;
+  line-height: 20px;
+  margin: -5px 3px;
+  padding: 0 $label-padding;
+  border: 1px solid $border-color;
+  border-radius: $label-border-radius;
+  font-weight: $gl-font-weight-normal;
+}

app/assets/stylesheets/pages/labels.scss

@@ -104,7 +104,7 @@
 }
 
 .color-label {
-  padding: 3px 7px;
+  padding: 3px $label-padding;
   border-radius: $label-border-radius;
 }
 

app/assets/stylesheets/pages/notes.scss

@@ -619,26 +619,17 @@ ul.notes {
 }
 
 .note-role {
+  margin: 0 3px;
+}
+
+.note-role-special {
   position: relative;
   display: inline-block;
-  color: $notes-role-color;
+  color: $gl-text-color-secondary;
   font-size: 12px;
-  line-height: 20px;
-  margin: 0 3px;
-
-  &.note-role-access {
-    padding: 0 7px;
-    border: 1px solid $border-color;
-    border-radius: $label-border-radius;
-  }
-
-  &.note-role-special {
-    text-shadow: 0 0 15px $gl-text-color-inverted;
-  }
+  text-shadow: 0 0 15px $gl-text-color-inverted;
 }
 
-
-
 /**
  * Line note button on the side of diffs
  */

app/controllers/concerns/renders_member_access.rb

+module RendersMemberAccess
+  def prepare_groups_for_rendering(groups)
+    preload_max_member_access_for_collection(Group, groups)
+
+    groups
+  end
+
+  def prepare_projects_for_rendering(projects)
+    preload_max_member_access_for_collection(Project, projects)
+
+    projects
+  end
+
+  private
+
+  def preload_max_member_access_for_collection(klass, collection)
+    return if !current_user || collection.blank?
+
+    method_name = "max_member_access_for_#{klass.name.underscore}_ids"
+
+    current_user.public_send(method_name, collection.ids) # rubocop:disable GitlabSecurity/PublicSend
+  end
+end

app/controllers/dashboard/projects_controller.rb

 class Dashboard::ProjectsController < Dashboard::ApplicationController
   include ParamsBackwardCompatibility
+  include RendersMemberAccess
 
   before_action :set_non_archived_param
   before_action :default_sorting
@@ -45,10 +46,12 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController
   end
 
   def load_projects(finder_params)
-    ProjectsFinder
-      .new(params: finder_params, current_user: current_user)
-      .execute
-      .includes(:route, :creator, namespace: [:route, :owner])
+    projects = ProjectsFinder
+                .new(params: finder_params, current_user: current_user)
+                .execute
+                .includes(:route, :creator, namespace: [:route, :owner])
+
+    prepare_projects_for_rendering(projects)
   end
 
   def load_events

app/controllers/explore/projects_controller.rb

 class Explore::ProjectsController < Explore::ApplicationController
   include ParamsBackwardCompatibility
+  include RendersMemberAccess
 
   before_action :set_non_archived_param
 
@@ -49,10 +50,12 @@ class Explore::ProjectsController < Explore::ApplicationController
   private
 
   def load_projects
-    ProjectsFinder.new(current_user: current_user, params: params)
-      .execute
-      .includes(:route, namespace: :route)
-      .page(params[:page])
-      .without_count
+    projects = ProjectsFinder.new(current_user: current_user, params: params)
+                 .execute
+                 .includes(:route, namespace: :route)
+                 .page(params[:page])
+                 .without_count
+
+    prepare_projects_for_rendering(projects)
   end
 end

app/controllers/users_controller.rb

 class UsersController < ApplicationController
   include RoutableActions
+  include RendersMemberAccess
 
   skip_before_action :authenticate_user!
   before_action :user, except: [:exists]
@@ -116,14 +117,20 @@ class UsersController < ApplicationController
     @projects =
       PersonalProjectsFinder.new(user).execute(current_user)
       .page(params[:page])
+
+    prepare_projects_for_rendering(@projects)
   end
 
   def load_contributed_projects
     @contributed_projects = contributed_projects.joined(user)
+
+    prepare_projects_for_rendering(@contributed_projects)
   end
 
   def load_groups
     @groups = JoinedGroupsFinder.new(user).execute(current_user)
+
+    prepare_groups_for_rendering(@groups)
   end
 
   def load_snippets

app/models/concerns/bulk_member_access_load.rb

+# Returns and caches in thread max member access for a resource
+#
+module BulkMemberAccessLoad
+  extend ActiveSupport::Concern
+
+  included do
+    # Determine the maximum access level for a group of resources in bulk.
+    #
+    # Returns a Hash mapping resource ID -> maximum access level.
+    def max_member_access_for_resource_ids(resource_klass, resource_ids, memoization_index = self.id, &block)
+      raise 'Block is mandatory' unless block_given?
+
+      resource_ids = resource_ids.uniq
+      key = max_member_access_for_resource_key(resource_klass, memoization_index)
+      access = {}
+
+      if RequestStore.active?
+        RequestStore.store[key] ||= {}
+        access = RequestStore.store[key]
+      end
+
+      # Look up only the IDs we need
+      resource_ids = resource_ids - access.keys
+
+      return access if resource_ids.empty?
+
+      resource_access = yield(resource_ids)
+
+      access.merge!(resource_access)
+
+      missing_resource_ids = resource_ids - resource_access.keys
+
+      missing_resource_ids.each do |resource_id|
+        access[resource_id] = Gitlab::Access::NO_ACCESS
+      end
+
+      access
+    end
+
+    private
+
+    def max_member_access_for_resource_key(klass, memoization_index)
+      "max_member_access_for_#{klass.name.underscore.pluralize}:#{memoization_index}"
+    end
+  end
+end

app/models/project_team.rb

 class ProjectTeam
+  include BulkMemberAccessLoad
+
   attr_accessor :project
 
   def initialize(project)
@@ -157,39 +159,16 @@ class ProjectTeam
   #
   # Returns a Hash mapping user ID -> maximum access level.
   def max_member_access_for_user_ids(user_ids)
-    user_ids = user_ids.uniq
-    key = "max_member_access:#{project.id}"
-
-    access = {}
-
-    if RequestStore.active?
-      RequestStore.store[key] ||= {}
-      access = RequestStore.store[key]
+    max_member_access_for_resource_ids(User, user_ids, project.id) do |user_ids|
+      project.project_authorizations
+             .where(user: user_ids)
+             .group(:user_id)
+             .maximum(:access_level)
     end
-
-    # Look up only the IDs we need
-    user_ids = user_ids - access.keys
-
-    return access if user_ids.empty?
-
-    users_access = project.project_authorizations
-      .where(user: user_ids)
-      .group(:user_id)
-      .maximum(:access_level)
-
-    access.merge!(users_access)
-
-    missing_user_ids = user_ids - users_access.keys
-
-    missing_user_ids.each do |user_id|
-      access[user_id] = Gitlab::Access::NO_ACCESS
-    end
-
-    access
   end
 
   def max_member_access(user_id)
-    max_member_access_for_user_ids([user_id])[user_id] || Gitlab::Access::NO_ACCESS
+    max_member_access_for_user_ids([user_id])[user_id]
   end
 
   private

app/models/user.rb

@@ -17,6 +17,7 @@ class User < ActiveRecord::Base
   include FeatureGate
   include CreatedAtFilterable
   include IgnorableColumn
+  include BulkMemberAccessLoad
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
@@ -1144,6 +1145,34 @@ class User < ActiveRecord::Base
     super
   end
 
+  # Determine the maximum access level for a group of projects in bulk.
+  #
+  # Returns a Hash mapping project ID -> maximum access level.
+  def max_member_access_for_project_ids(project_ids)
+    max_member_access_for_resource_ids(Project, project_ids) do |project_ids|
+      project_authorizations.where(project: project_ids)
+                            .group(:project_id)
+                            .maximum(:access_level)
+    end
+  end
+
+  def max_member_access_for_project(project_id)
+    max_member_access_for_project_ids([project_id])[project_id]
+  end
+
+  # Determine the maximum access level for a group of groups in bulk.
+  #
+  # Returns a Hash mapping project ID -> maximum access level.
+  def max_member_access_for_group_ids(group_ids)
+    max_member_access_for_resource_ids(Group, group_ids) do |group_ids|
+      group_members.where(source: group_ids).group(:source_id).maximum(:access_level)
+    end
+  end
+
+  def max_member_access_for_group(group_id)
+    max_member_access_for_group_ids([group_id])[group_id]
+  end
+
   protected
 
   # override, from Devise::Validatable

app/views/dashboard/projects/_projects.html.haml

-= render 'shared/projects/list', projects: @projects, ci: true
+= render 'shared/projects/list', projects: @projects, ci: true, user: current_user

app/views/explore/projects/_projects.html.haml

-= render 'shared/projects/list', projects: projects
+= render 'shared/projects/list', projects: projects, user: current_user

app/views/projects/notes/_actions.html.haml

@@ -3,7 +3,7 @@
   %span.note-role.note-role-special.has-tooltip{ title: _("This is the author's first Merge Request to this project.") }
     = issuable_first_contribution_icon
 - if access.nonzero?
-  %span.note-role.note-role-access= Gitlab::Access.human_access(access)
+  %span.note-role.user-access-role= Gitlab::Access.human_access(access)
 
 - if note.resolvable?
   - can_resolve = can?(current_user, :resolve_note, note)

app/views/shared/groups/_group.html.haml

-- group_member = local_assigns[:group_member]
-- full_name = true unless local_assigns[:full_name] == false
-- group_name = full_name ? group.full_name : group.name
-- css_class = '' unless local_assigns[:css_class]
-- css_class += " no-description" if group.description.blank?
-
-%li.group-row{ class: css_class }
-  - if group_member
-    .controls.hidden-xs
-      - if can?(current_user, :admin_group, group)
-        = link_to edit_group_path(group), class: "btn" do
-          = sprite_icon('settings')
-
-      = link_to leave_group_group_members_path(group), data: { confirm: leave_confirmation_message(group) }, method: :delete, class: "btn", title: s_("GroupsTree|Leave this group") do
-        = icon('sign-out')
+- user = local_assigns.fetch(:user, current_user)
+- access = user&.max_member_access_for_group(group.id)
 
+%li.group-row{ class: ('no-description' if group.description.blank?) }
   .stats
     %span
       = icon('bookmark')
@@ -30,11 +18,10 @@
     = link_to group do
       = group_icon(group, class: "avatar s40 hidden-xs")
   .title
-    = link_to group_name, group, class: 'group-name'
+    = link_to group.full_name, group, class: 'group-name'
 
-    - if group_member
-      as
-      %span= group_member.human_access
+    - if access&.nonzero?
+      %span.user-access-role= Gitlab::Access.human_access(access)
 
   - if group.description.present?
     .description

app/views/shared/groups/_list.html.haml

 - if groups.any?
+  - user = local_assigns[:user]
+
   %ul.content-list
     - groups.each_with_index do |group, i|
-      = render "shared/groups/group", group: group
+      = render "shared/groups/group", group: group, user: user
 - else
   .nothing-here-block= s_("GroupsEmptyState|No groups found")

app/views/shared/projects/_list.html.haml

@@ -5,18 +5,20 @@
 - forks = false unless local_assigns[:forks] == true
 - ci = false unless local_assigns[:ci] == true
 - skip_namespace = false unless local_assigns[:skip_namespace] == true
+- user = local_assigns[:user]
 - show_last_commit_as_description = false unless local_assigns[:show_last_commit_as_description] == true
 - remote = false unless local_assigns[:remote] == true
-- load_pipeline_status(projects)
 
 .js-projects-list-holder
   - if any_projects?(projects)
+    - load_pipeline_status(projects)
+
     %ul.projects-list
       - projects.each_with_index do |project, i|
         - css_class = (i >= projects_limit) || project.pending_delete? ? 'hide' : nil
         = render "shared/projects/project", project: project, skip_namespace: skip_namespace,
           avatar: avatar, stars: stars, css_class: css_class, ci: ci, use_creator_avatar: use_creator_avatar,
-          forks: forks, show_last_commit_as_description: show_last_commit_as_description
+          forks: forks, show_last_commit_as_description: show_last_commit_as_description, user: user
 
       - if @private_forks_count && @private_forks_count > 0
         %li.project-row.private-forks-notice

app/views/shared/projects/_project.html.haml

@@ -3,6 +3,8 @@
 - forks = false unless local_assigns[:forks] == true
 - ci = false unless local_assigns[:ci] == true
 - skip_namespace = false unless local_assigns[:skip_namespace] == true
+- user = local_assigns[:user]
+- access = user&.max_member_access_for_project(project.id) unless user.nil?
 - css_class = '' unless local_assigns[:css_class]
 - show_last_commit_as_description = false unless local_assigns[:show_last_commit_as_description] == true && project.commit
 - css_class += " no-description" if project.description.blank? && !show_last_commit_as_description
@@ -21,14 +23,19 @@
     .project-details
       %h3.prepend-top-0.append-bottom-0
         = link_to project_path(project), class: 'text-plain' do
-          %span.project-full-name
+          %span.project-full-name><
             %span.namespace-name
               - if project.namespace && !skip_namespace
                 = project.namespace.human_name
                 \/
-            %span.project-name
+            %span.project-name<
               = project.name
 
+        - if access&.nonzero?
+          -# haml-lint:disable UnnecessaryStringOutput
+          = ' ' # prevent haml from eating the space between elements
+          %span.user-access-role= Gitlab::Access.human_access(access)
+
       - if show_last_commit_as_description
         .description.prepend-top-5
           = link_to_markdown(project.commit.title, project_commit_path(project, project.commit), class: "commit-row-message")

spec/models/user_spec.rb

@@ -2433,4 +2433,163 @@ describe User do
       expect(user).not_to be_blocked
     end
   end
+
+  describe '#max_member_access_for_project_ids' do
+    shared_examples 'max member access for projects' do
+      let(:user) { create(:user) }
+      let(:group) { create(:group) }
+      let(:owner_project) { create(:project, group: group) }
+      let(:master_project) { create(:project) }
+      let(:reporter_project) { create(:project) }
+      let(:developer_project) { create(:project) }
+      let(:guest_project) { create(:project) }
+      let(:no_access_project) { create(:project) }
+
+      let(:projects) do
+        [owner_project, master_project, reporter_project, developer_project, guest_project, no_access_project].map(&:id)
+      end
+
+      let(:expected) do
+        {
+          owner_project.id => Gitlab::Access::OWNER,
+          master_project.id => Gitlab::Access::MASTER,
+          reporter_project.id => Gitlab::Access::REPORTER,
+          developer_project.id => Gitlab::Access::DEVELOPER,
+          guest_project.id => Gitlab::Access::GUEST,
+          no_access_project.id => Gitlab::Access::NO_ACCESS
+        }
+      end
+
+      before do
+        create(:group_member, user: user, group: group)
+        master_project.add_master(user)
+        reporter_project.add_reporter(user)
+        developer_project.add_developer(user)
+        guest_project.add_guest(user)
+      end
+
+      it 'returns correct roles for different projects' do
+        expect(user.max_member_access_for_project_ids(projects)).to eq(expected)
+      end
+    end
+
+    context 'with RequestStore enabled', :request_store do
+      include_examples 'max member access for projects'
+
+      def access_levels(projects)
+        user.max_member_access_for_project_ids(projects)
+      end
+
+      it 'does not perform extra queries when asked for projects who have already been found' do
+        access_levels(projects)
+
+        expect { access_levels(projects) }.not_to exceed_query_limit(0)
+
+        expect(access_levels(projects)).to eq(expected)
+      end
+
+      it 'only requests the extra projects when uncached projects are passed' do
+        second_master_project = create(:project)
+        second_developer_project = create(:project)
+        second_master_project.add_master(user)
+        second_developer_project.add_developer(user)
+
+        all_projects = projects + [second_master_project.id, second_developer_project.id]
+
+        expected_all = expected.merge(second_master_project.id => Gitlab::Access::MASTER,
+                                      second_developer_project.id => Gitlab::Access::DEVELOPER)
+
+        access_levels(projects)
+
+        queries = ActiveRecord::QueryRecorder.new { access_levels(all_projects) }
+
+        expect(queries.count).to eq(1)
+        expect(queries.log_message).to match(/\W(#{second_master_project.id}, #{second_developer_project.id})\W/)
+        expect(access_levels(all_projects)).to eq(expected_all)
+      end
+    end
+
+    context 'with RequestStore disabled' do
+      include_examples 'max member access for projects'
+    end
+  end
+
+  describe '#max_member_access_for_group_ids' do
+    shared_examples 'max member access for groups' do
+      let(:user) { create(:user) }
+      let(:owner_group) { create(:group) }
+      let(:master_group) { create(:group) }
+      let(:reporter_group) { create(:group) }
+      let(:developer_group) { create(:group) }
+      let(:guest_group) { create(:group) }
+      let(:no_access_group) { create(:group) }
+
+      let(:groups) do
+        [owner_group, master_group, reporter_group, developer_group, guest_group, no_access_group].map(&:id)
+      end
+
+      let(:expected) do
+        {
+          owner_group.id => Gitlab::Access::OWNER,
+          master_group.id => Gitlab::Access::MASTER,
+          reporter_group.id => Gitlab::Access::REPORTER,
+          developer_group.id => Gitlab::Access::DEVELOPER,
+          guest_group.id => Gitlab::Access::GUEST,
+          no_access_group.id => Gitlab::Access::NO_ACCESS
+        }
+      end
+
+      before do
+        owner_group.add_owner(user)
+        master_group.add_master(user)
+        reporter_group.add_reporter(user)
+        developer_group.add_developer(user)
+        guest_group.add_guest(user)
+      end
+
+      it 'returns correct roles for different groups' do
+        expect(user.max_member_access_for_group_ids(groups)).to eq(expected)
+      end
+    end
+
+    context 'with RequestStore enabled', :request_store do
+      include_examples 'max member access for groups'
+
+      def access_levels(groups)
+        user.max_member_access_for_group_ids(groups)
+      end
+
+      it 'does not perform extra queries when asked for groups who have already been found' do
+        access_levels(groups)
+
+        expect { access_levels(groups) }.not_to exceed_query_limit(0)
+
+        expect(access_levels(groups)).to eq(expected)
+      end
+
+      it 'only requests the extra groups when uncached groups are passed' do
+        second_master_group = create(:group)
+        second_developer_group = create(:group)
+        second_master_group.add_master(user)
+        second_developer_group.add_developer(user)
+
+        all_groups = groups + [second_master_group.id, second_developer_group.id]
+
+        expected_all = expected.merge(second_master_group.id => Gitlab::Access::MASTER,
+                                      second_developer_group.id => Gitlab::Access::DEVELOPER)
+
+        access_levels(groups)
+
+        queries = ActiveRecord::QueryRecorder.new { access_levels(all_groups) }
+
+        expect(queries.count).to eq(1)
+        expect(queries.log_message).to match(/\W(#{second_master_group.id}, #{second_developer_group.id})\W/)
+        expect(access_levels(all_groups)).to eq(expected_all)
+      end
+    end
+
+    context 'with RequestStore disabled' do
+      include_examples 'max member access for groups'
+    end
+  end
 end