Merge branch 'sh-break-out-invited-group-members' into 'master'

Make it easier to find invited group members

Closes #61948

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

app/assets/stylesheets/pages/members.scss

@@ -67,7 +67,7 @@
   position: absolute;
   right: 4px;
   top: 0;
-  height: 35px;
+  height: $input-height;
   padding-left: 10px;
   padding-right: 10px;
   color: $gray-darkest;

app/controllers/groups/group_members_controller.rb

@@ -5,6 +5,8 @@ class Groups::GroupMembersController < Groups::ApplicationController
   include MembersPresentation
   include SortingHelper
 
+  MEMBER_PER_PAGE_LIMIT = 50
+
   def self.admin_not_required_endpoints
     %i[index leave request_access]
   end
@@ -24,7 +26,14 @@ class Groups::GroupMembersController < Groups::ApplicationController
     @project = @group.projects.find(params[:project_id]) if params[:project_id]
 
     @members = GroupMembersFinder.new(@group).execute
-    @members = @members.non_invite unless can_manage_members
+
+    if can_manage_members
+      @invited_members = @members.invite
+      @invited_members = @invited_members.search_invite_email(params[:search_invited]) if params[:search_invited].present?
+      @invited_members = present_members(@invited_members.page(params[:invited_members_page]).per(MEMBER_PER_PAGE_LIMIT))
+    end
+
+    @members = @members.non_invite
     @members = @members.search(params[:search]) if params[:search].present?
     @members = @members.sort_by_attribute(@sort)
 
@@ -32,7 +41,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
       @members = @members.filter_by_2fa(params[:two_factor])
     end
 
-    @members = @members.page(params[:page]).per(50)
+    @members = @members.page(params[:page]).per(MEMBER_PER_PAGE_LIMIT)
     @members = present_members(@members)
 
     @requesters = present_members(

app/models/member.rb

@@ -107,6 +107,10 @@ class Member < ApplicationRecord
       joins(:user).merge(User.search(query))
     end
 
+    def search_invite_email(query)
+      invite.where(['invite_email ILIKE ?', "%#{query}%"])
+    end
+
     def filter_by_2fa(value)
       case value
       when 'enabled'

app/views/groups/group_members/index.html.haml

-- page_title "Members"
+- page_title _("Members")
 - can_manage_members = can?(current_user, :admin_group_member, @group)
+- show_invited_members = can_manage_members && @invited_members.exists?
+- pending_active = params[:search_invited].present?
 
 .project-members-page.prepend-top-default
   %h4
-    Members
+    = _("Members")
   %hr
   - if can_manage_members
     .project-members-new.append-bottom-default
       %p.clearfix
-        Add new member to
-        %strong= @group.name
+        = _("Add new member to %{strong_start}%{group_name}%{strong_end}").html_safe % { group_name: @group.name, strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe }
       = render "new_group_member"
 
     = render 'shared/members/requests', membership_source: @group, requesters: @requesters
 
   = render_if_exists 'groups/group_members/ldap_sync'
 
-  .clearfix
-    %h5.member.existing-title
-      Existing members
-  .card
-    .card-header.flex-project-members-panel
-      %span.flex-project-title
-        Members with access to
-        %strong= @group.name
+  %ul.nav-links.mobile-separator.nav.nav-tabs.clearfix
+    %li.nav-item
+      = link_to "#existing_members", class: ["nav-link", ("active" unless pending_active)] , 'data-toggle' => 'tab' do
+        %span
+          = _("Existing")
         %span.badge.badge-pill= @members.total_count
-      = form_tag group_group_members_path(@group), method: :get, class: 'form-inline user-search-form flex-users-form' do
-        .form-group
-          .position-relative.append-right-8
-            = search_field_tag :search, params[:search], { placeholder: 'Find existing members by name', class: 'form-control', spellcheck: false }
-            %button.user-search-btn{ type: "submit", "aria-label" => "Submit search" }
+    - if show_invited_members
+      %li.nav-item
+        = link_to "#invited_members", class: ["nav-link", ("active" if pending_active)], 'data-toggle' => 'tab' do
+          %span
+            = _("Pending")
+          %span.badge.badge-pill= @invited_members.total_count
+
+  .tab-content
+    #existing_members.tab-pane{ :class => ("active" unless pending_active) }
+      .card.card-without-border
+        .d-flex.flex-column.flex-md-row.row-content-block.second-block
+          %span.flex-grow-1.align-self-md-center.col-form-label
+            = _("Members with access to %{strong_start}%{group_name}%{strong_end}").html_safe % { group_name: @group.name, strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe }
+          = form_tag group_group_members_path(@group), method: :get, class: 'form-inline user-search-form' do
+            .form-group.flex-grow
+              .position-relative.mr-md-2
+                = search_field_tag :search, params[:search], { placeholder: _('Search'), class: 'form-control', spellcheck: false }
+                %button.user-search-btn.border-left{ type: "submit", "aria-label" => _("Submit search") }
                   = icon("search")
               - if can_manage_members
+                = label_tag '2fa', '2FA', class: 'col-form-label label-bold pr-md-2'
                 = render 'shared/members/filter_2fa_dropdown'
               = render 'shared/members/sort_dropdown'
         %ul.content-list.members-list
           = render partial: 'shared/members/member', collection: @members, as: :member
       = paginate @members, theme: 'gitlab'
+
+    - if show_invited_members
+      #invited_members.tab-pane{ :class => ("active" if pending_active) }
+        .card.card-without-border
+          .d-flex.flex-column.flex-md-row.row-content-block.second-block
+            %span.flex-grow-1
+              = _("Members with pending access to %{strong_start}%{group_name}%{strong_end}").html_safe % { group_name: @group.name, strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe }
+            = form_tag group_group_members_path(@group), method: :get, class: 'form-inline user-search-form' do
+              .form-group
+                .position-relative.mr-md-2
+                  = search_field_tag :search_invited, params[:search_invited], { placeholder: _('Search'), class: 'form-control', spellcheck: false }
+                  %button.user-search-btn.border-left{ type: "submit", "aria-label" => _("Submit search") }
+                    = icon("search")
+          %ul.content-list.members-list
+            = render partial: 'shared/members/member', collection: @invited_members, as: :member
+        = paginate @invited_members, param_name: 'invited_members_page', theme: 'gitlab'

app/views/shared/members/_filter_2fa_dropdown.html.haml

 - filter = params[:two_factor] || 'everyone'
 - filter_options = { 'everyone' => _('Everyone'), 'enabled' => _('Enabled'), 'disabled' => _('Disabled') }
-.dropdown.inline.member-filter-2fa-dropdown
-  = dropdown_toggle('2FA: ' + filter_options[filter], { toggle: 'dropdown' })
+.dropdown.inline.member-filter-2fa-dropdown.pr-md-2
+  = dropdown_toggle(filter_options[filter], { toggle: 'dropdown' })
   %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-selectable
     %li.dropdown-header
       = _("Filter by two-factor authentication")

app/views/shared/members/_sort_dropdown.html.haml

-.dropdown.inline.user-sort-dropdown
+= label_tag :sort_by, 'Sort by', class: 'col-form-label label-bold pr-2'
+.dropdown.inline.qa-user-sort-dropdown
   = dropdown_toggle(member_sort_options_hash[@sort], { toggle: 'dropdown' })
   %ul.dropdown-menu.dropdown-menu-right.dropdown-menu-selectable
     %li.dropdown-header

changelogs/unreleased/sh-break-out-invited-group-members.yml

+---
+title: Make it easier to find invited group members
+merge_request: 28436
+author:
+type: fixed

locale/gitlab.pot

@@ -714,6 +714,9 @@ msgstr ""
 msgid "Add new directory"
 msgstr ""
 
+msgid "Add new member to %{strong_start}%{group_name}%{strong_end}"
+msgstr ""
+
 msgid "Add or subtract spent time"
 msgstr ""
 
@@ -4643,6 +4646,9 @@ msgstr ""
 msgid "Except policy:"
 msgstr ""
 
+msgid "Existing"
+msgstr ""
+
 msgid "Existing members and groups"
 msgstr ""
 
@@ -6625,6 +6631,12 @@ msgstr ""
 msgid "Members of <strong>%{project_name}</strong>"
 msgstr ""
 
+msgid "Members with access to %{strong_start}%{group_name}%{strong_end}"
+msgstr ""
+
+msgid "Members with pending access to %{strong_start}%{group_name}%{strong_end}"
+msgstr ""
+
 msgid "Merge"
 msgstr ""
 

spec/controllers/groups/group_members_controller_spec.rb

@@ -16,6 +16,39 @@ describe Groups::GroupMembersController do
       expect(response).to have_gitlab_http_status(200)
       expect(response).to render_template(:index)
     end
+
+    context 'user with owner access' do
+      let!(:invited) { create_list(:group_member, 3, :invited, group: group) }
+
+      before do
+        group.add_owner(user)
+        sign_in(user)
+      end
+
+      it 'assigns invited members' do
+        get :index, params: { group_id: group }
+
+        expect(assigns(:invited_members).map(&:invite_email)).to match_array(invited.map(&:invite_email))
+      end
+
+      it 'restricts search to one email' do
+        get :index, params: { group_id: group, search_invited: invited.first.invite_email }
+
+        expect(assigns(:invited_members).map(&:invite_email)).to match_array(invited.first.invite_email)
+      end
+
+      it 'paginates invited list' do
+        stub_const('Groups::GroupMembersController::MEMBER_PER_PAGE_LIMIT', 2)
+
+        get :index, params: { group_id: group, invited_members_page: 1 }
+
+        expect(assigns(:invited_members).count).to eq(2)
+
+        get :index, params: { group_id: group, invited_members_page: 2 }
+
+        expect(assigns(:invited_members).count).to eq(1)
+      end
+    end
   end
 
   describe 'POST create' do

spec/factories/group_members.rb

@@ -16,7 +16,9 @@ FactoryBot.define do
     trait(:invited) do
       user_id nil
       invite_token 'xxx'
-      invite_email 'email@email.com'
+      sequence :invite_email do |n|
+        "email#{n}@email.com"
+      end
     end
   end
 end

spec/features/groups/members/filter_members_spec.rb

@@ -19,7 +19,7 @@ describe 'Groups > Members > Filter members' do
 
     expect(first_member).to include(user.name)
     expect(second_member).to include(user_with_2fa.name)
-    expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: '2FA: Everyone')
+    expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: 'Everyone')
   end
 
   it 'shows only 2FA members' do
@@ -27,7 +27,7 @@ describe 'Groups > Members > Filter members' do
 
     expect(first_member).to include(user_with_2fa.name)
     expect(members_list.size).to eq(1)
-    expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: '2FA: Enabled')
+    expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: 'Enabled')
   end
 
   it 'shows only non 2FA members' do
@@ -35,7 +35,7 @@ describe 'Groups > Members > Filter members' do
 
     expect(first_member).to include(user.name)
     expect(members_list.size).to eq(1)
-    expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: '2FA: Disabled')
+    expect(page).to have_css('.member-filter-2fa-dropdown .dropdown-toggle-text', text: 'Disabled')
   end
 
   def visit_members_list(options = {})

spec/features/groups/members/manage_members_spec.rb

@@ -98,7 +98,9 @@ describe 'Groups > Members > Manage members' do
 
     add_user('test@example.com', 'Reporter')
 
-    page.within(second_row) do
+    click_link('Pending')
+
+    page.within('.content-list.members-list') do
       expect(page).to have_content('test@example.com')
       expect(page).to have_content('Invited')
       expect(page).to have_button('Reporter')

spec/features/groups/members/sort_members_spec.rb

@@ -19,7 +19,7 @@ describe 'Groups > Members > Sort members' do
 
     expect(first_member).to include(owner.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Name, ascending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Name, ascending')
   end
 
   it 'sorts by access level ascending' do
@@ -27,7 +27,7 @@ describe 'Groups > Members > Sort members' do
 
     expect(first_member).to include(developer.name)
     expect(second_member).to include(owner.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Access level, ascending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Access level, ascending')
   end
 
   it 'sorts by access level descending' do
@@ -35,7 +35,7 @@ describe 'Groups > Members > Sort members' do
 
     expect(first_member).to include(owner.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Access level, descending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Access level, descending')
   end
 
   it 'sorts by last joined' do
@@ -43,7 +43,7 @@ describe 'Groups > Members > Sort members' do
 
     expect(first_member).to include(developer.name)
     expect(second_member).to include(owner.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Last joined')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Last joined')
   end
 
   it 'sorts by oldest joined' do
@@ -51,7 +51,7 @@ describe 'Groups > Members > Sort members' do
 
     expect(first_member).to include(owner.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Oldest joined')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Oldest joined')
   end
 
   it 'sorts by name ascending' do
@@ -59,7 +59,7 @@ describe 'Groups > Members > Sort members' do
 
     expect(first_member).to include(owner.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Name, ascending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Name, ascending')
   end
 
   it 'sorts by name descending' do
@@ -67,7 +67,7 @@ describe 'Groups > Members > Sort members' do
 
     expect(first_member).to include(developer.name)
     expect(second_member).to include(owner.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Name, descending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Name, descending')
   end
 
   it 'sorts by recent sign in', :clean_gitlab_redis_shared_state do
@@ -75,7 +75,7 @@ describe 'Groups > Members > Sort members' do
 
     expect(first_member).to include(owner.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Recent sign in')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Recent sign in')
   end
 
   it 'sorts by oldest sign in', :clean_gitlab_redis_shared_state do
@@ -83,7 +83,7 @@ describe 'Groups > Members > Sort members' do
 
     expect(first_member).to include(developer.name)
     expect(second_member).to include(owner.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Oldest sign in')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Oldest sign in')
   end
 
   def visit_members_list(sort:)

spec/features/projects/members/sorting_spec.rb

@@ -18,7 +18,7 @@ describe 'Projects > Members > Sorting' do
 
     expect(first_member).to include(maintainer.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Name, ascending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Name, ascending')
   end
 
   it 'sorts by access level ascending' do
@@ -26,7 +26,7 @@ describe 'Projects > Members > Sorting' do
 
     expect(first_member).to include(developer.name)
     expect(second_member).to include(maintainer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Access level, ascending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Access level, ascending')
   end
 
   it 'sorts by access level descending' do
@@ -34,7 +34,7 @@ describe 'Projects > Members > Sorting' do
 
     expect(first_member).to include(maintainer.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Access level, descending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Access level, descending')
   end
 
   it 'sorts by last joined' do
@@ -42,7 +42,7 @@ describe 'Projects > Members > Sorting' do
 
     expect(first_member).to include(maintainer.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Last joined')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Last joined')
   end
 
   it 'sorts by oldest joined' do
@@ -50,7 +50,7 @@ describe 'Projects > Members > Sorting' do
 
     expect(first_member).to include(developer.name)
     expect(second_member).to include(maintainer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Oldest joined')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Oldest joined')
   end
 
   it 'sorts by name ascending' do
@@ -58,7 +58,7 @@ describe 'Projects > Members > Sorting' do
 
     expect(first_member).to include(maintainer.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Name, ascending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Name, ascending')
   end
 
   it 'sorts by name descending' do
@@ -66,7 +66,7 @@ describe 'Projects > Members > Sorting' do
 
     expect(first_member).to include(developer.name)
     expect(second_member).to include(maintainer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Name, descending')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Name, descending')
   end
 
   it 'sorts by recent sign in', :clean_gitlab_redis_shared_state do
@@ -74,7 +74,7 @@ describe 'Projects > Members > Sorting' do
 
     expect(first_member).to include(maintainer.name)
     expect(second_member).to include(developer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Recent sign in')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Recent sign in')
   end
 
   it 'sorts by oldest sign in', :clean_gitlab_redis_shared_state do
@@ -82,7 +82,7 @@ describe 'Projects > Members > Sorting' do
 
     expect(first_member).to include(developer.name)
     expect(second_member).to include(maintainer.name)
-    expect(page).to have_css('.user-sort-dropdown .dropdown-toggle-text', text: 'Oldest sign in')
+    expect(page).to have_css('.qa-user-sort-dropdown .dropdown-toggle-text', text: 'Oldest sign in')
   end
 
   def visit_members_list(sort:)

spec/models/member_spec.rb

@@ -172,6 +172,19 @@ describe Member do
       it { expect(described_class.non_request).to include @accepted_request_member }
     end
 
+    describe '.search_invite_email' do
+      it 'returns only members the matching e-mail' do
+        create(:group_member, :invited)
+
+        invited = described_class.search_invite_email(@invited_member.invite_email)
+
+        expect(invited.count).to eq(1)
+        expect(invited.first).to eq(@invited_member)
+
+        expect(described_class.search_invite_email('bad-email@example.com').count).to eq(0)
+      end
+    end
+
     describe '.developers' do
       subject { described_class.developers.to_a }