diff options
author | Phil Hughes <me@iamphill.com> | 2017-12-07 09:11:42 +0000 |
---|---|---|
committer | Phil Hughes <me@iamphill.com> | 2017-12-07 09:11:42 +0000 |
commit | 222df36c1576a96a784f11818689297a00cdf4b4 (patch) | |
tree | cbad93902334c10575d3e6ad1cd1f8903fa8c6f4 | |
parent | 61bd5b2cdb19e78e5f3f5688dc6fc2668e8a5608 (diff) | |
parent | 2c569be63b3275c311606e4afcfb8311e810401f (diff) | |
download | gitlab-ce-222df36c1576a96a784f11818689297a00cdf4b4.tar.gz |
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
22 files changed, 347 insertions, 94 deletions
diff --git a/app/assets/javascripts/groups/components/group_item.vue b/app/assets/javascripts/groups/components/group_item.vue index 0cd0c59a275..c76ce762b54 100644 --- a/app/assets/javascripts/groups/components/group_item.vue +++ b/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" diff --git a/app/assets/javascripts/notes/components/note_actions.vue b/app/assets/javascripts/notes/components/note_actions.vue index 45fc6196be4..7fb45ed4d4b 100644 --- a/app/assets/javascripts/notes/components/note_actions.vue +++ b/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"> diff --git a/app/assets/stylesheets/framework/lists.scss b/app/assets/stylesheets/framework/lists.scss index cd505bcaf6d..e6e6c4c3963 100644 --- a/app/assets/stylesheets/framework/lists.scss +++ b/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; } } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 6525b39d55c..4f99c27eff1 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/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); diff --git a/app/assets/stylesheets/pages/groups.scss b/app/assets/stylesheets/pages/groups.scss index 9b7dda9b648..f9a761e85fe 100644 --- a/app/assets/stylesheets/pages/groups.scss +++ b/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; +} diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index 443f5500684..92abe82df4c 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -104,7 +104,7 @@ } .color-label { - padding: 3px 7px; + padding: 3px $label-padding; border-radius: $label-border-radius; } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index a6009ab328e..4d5613c292b 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/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 */ diff --git a/app/controllers/concerns/renders_member_access.rb b/app/controllers/concerns/renders_member_access.rb new file mode 100644 index 00000000000..d640378c24d --- /dev/null +++ b/app/controllers/concerns/renders_member_access.rb @@ -0,0 +1,23 @@ +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 diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index d9884a47ec4..de9f8f9224a 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -1,5 +1,6 @@ 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 diff --git a/app/controllers/explore/projects_controller.rb b/app/controllers/explore/projects_controller.rb index 762c6ebf3a3..c7273606a85 100644 --- a/app/controllers/explore/projects_controller.rb +++ b/app/controllers/explore/projects_controller.rb @@ -1,5 +1,6 @@ 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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 5fca31b4956..575ec5c20f0 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,5 +1,6 @@ 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 diff --git a/app/models/concerns/bulk_member_access_load.rb b/app/models/concerns/bulk_member_access_load.rb new file mode 100644 index 00000000000..984c4f53bf7 --- /dev/null +++ b/app/models/concerns/bulk_member_access_load.rb @@ -0,0 +1,46 @@ +# 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 diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 1d35426050e..c679758973a 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -1,4 +1,6 @@ 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 diff --git a/app/models/user.rb b/app/models/user.rb index 38ee4ed50c1..af1c36d9c93 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/views/dashboard/projects/_projects.html.haml b/app/views/dashboard/projects/_projects.html.haml index 0ebd7c01bab..9e0e908e656 100644 --- a/app/views/dashboard/projects/_projects.html.haml +++ b/app/views/dashboard/projects/_projects.html.haml @@ -1 +1 @@ -= render 'shared/projects/list', projects: @projects, ci: true += render 'shared/projects/list', projects: @projects, ci: true, user: current_user diff --git a/app/views/explore/projects/_projects.html.haml b/app/views/explore/projects/_projects.html.haml index 708fbc27f55..67f2f897137 100644 --- a/app/views/explore/projects/_projects.html.haml +++ b/app/views/explore/projects/_projects.html.haml @@ -1 +1 @@ -= render 'shared/projects/list', projects: projects += render 'shared/projects/list', projects: projects, user: current_user diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml index 4961835f12a..5ea653ccad5 100644 --- a/app/views/projects/notes/_actions.html.haml +++ b/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) diff --git a/app/views/shared/groups/_group.html.haml b/app/views/shared/groups/_group.html.haml index 321d8767d08..90395600d4e 100644 --- a/app/views/shared/groups/_group.html.haml +++ b/app/views/shared/groups/_group.html.haml @@ -1,19 +1,7 @@ -- 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 diff --git a/app/views/shared/groups/_list.html.haml b/app/views/shared/groups/_list.html.haml index aec8ecd1714..f50a6bd4d6a 100644 --- a/app/views/shared/groups/_list.html.haml +++ b/app/views/shared/groups/_list.html.haml @@ -1,6 +1,8 @@ - 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") diff --git a/app/views/shared/projects/_list.html.haml b/app/views/shared/projects/_list.html.haml index 0bedfea3502..e1da05d8f08 100644 --- a/app/views/shared/projects/_list.html.haml +++ b/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 diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 98bfc7c4d36..003f5fa52eb 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/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") diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b27c1b2cd1a..03c96a8f5aa 100644 --- a/spec/models/user_spec.rb +++ b/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 |