Merge branch '229830-move-groups-members-pending-in-to-tabs' into 'master'

Reorganize group member management into tabs

See merge request gitlab-org/gitlab!38344

app/assets/stylesheets/pages/users.scss

@@ -25,8 +25,12 @@
   }
 
   .form-control {
-    width: 100%;
     padding-right: 35px;
+  }
+
+  .search-control-wrap,
+  .form-control {
+    width: 100%;
 
     @include media-breakpoint-up(sm) {
       width: 250px;

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

-- page_title _("Group members")
+- page_title _('Group 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?
-- total_count = @members.count + @group.shared_with_group_links.count
+- show_access_requests = can_manage_members && @requesters.exists?
+- invited_active = params[:search_invited].present? || params[:invited_members_page].present?
+
+- form_item_label_css_class = 'label-bold gl-mr-2 gl-mb-0 gl-py-2 align-self-md-center'
 
 .js-remove-member-modal
 .project-members-page.gl-mt-3
   %h4
-    = _("Group members")
+    = _('Group members')
   %hr
   - if can_manage_members
     %ul.nav-links.nav.nav-tabs.gitlab-tabs{ role: 'tablist' }
       %li.nav-tab{ role: 'presentation' }
-        %a.nav-link.active{ href: '#invite-member-pane', id: 'invite-member-tab', data: { toggle: 'tab' }, role: 'tab' }= _("Invite member")
+        %a.nav-link.active{ href: '#invite-member-pane', id: 'invite-member-tab', data: { toggle: 'tab' }, role: 'tab' }= _('Invite member')
         %li.nav-tab{ role: 'presentation' }
-          %a.nav-link{ href: '#invite-group-pane', id: 'invite-group-tab', data: { toggle: 'tab', qa_selector: 'invite_group_tab' }, role: 'tab' }= _("Invite group")
+          %a.nav-link{ href: '#invite-group-pane', id: 'invite-group-tab', data: { toggle: 'tab', qa_selector: 'invite_group_tab' }, role: 'tab' }= _('Invite group')
     .tab-content.gitlab-tab-content
       .tab-pane.active{ id: 'invite-member-pane', role: 'tabpanel' }
         = render_invite_member_for_group(@group, @group_member.access_level)
       .tab-pane{ id: 'invite-group-pane', role: 'tabpanel' }
         = render 'shared/members/invite_group', submit_url: group_group_links_path(@group), access_levels: GroupMember.access_level_roles, default_access_level: @group_member.access_level, group_link_field: 'shared_with_group_id', group_access_field: 'shared_group_access'
 
-    = render 'shared/members/requests', membership_source: @group, requesters: @requesters
-
   = render_if_exists 'groups/group_members/ldap_sync'
 
-  %ul.nav-links.mobile-separator.nav.nav-tabs.clearfix
+  %ul.nav-links.mobile-separator.nav.nav-tabs
+    %li.nav-item
+      = link_to '#tab-members', class: ['nav-link', ('active' unless invited_active)], data: { toggle: 'tab' } do
+        %span
+          = _('Members')
+        %span.badge.badge-pill= @members.total_count
+    - if @group.shared_with_group_links.any?
       %li.nav-item
-      = link_to "#existing_shares", class: ["nav-link", ("active" unless pending_active)] , 'data-toggle' => 'tab' do
+        = link_to '#tab-groups', class: ['nav-link'] , data: { toggle: 'tab', qa_selector: 'groups_list_tab' } do
           %span
-          = _("Existing shares")
-        %span.badge.badge-pill= total_count
+            = _('Groups')
+          %span.badge.badge-pill= @group.shared_with_group_links.count
     - if show_invited_members
       %li.nav-item
-        = link_to "#invited_members", class: ["nav-link", ("active" if pending_active)], 'data-toggle' => 'tab' do
+        = link_to '#tab-invited-members', class: ['nav-link', ('active' if invited_active)], data: { toggle: 'tab' } do
           %span
-            = _("Pending")
+            = _('Invited')
           %span.badge.badge-pill= @invited_members.total_count
-
+    - if show_access_requests
+      %li.nav-item
+        = link_to '#tab-access-requests', class: 'nav-link', data: { toggle: 'tab' } do
+          %span
+            = _('Access requests')
+          %span.badge.badge-pill= @requesters.count
   .tab-content
-    #existing_shares.tab-pane{ :class => ("active" unless pending_active) }
-      - if @group.shared_with_group_links.any?
+    #tab-members.tab-pane{ class: ('active' unless invited_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
-              = _("Groups 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 }
-          %ul.content-list.members-list{ data: { qa_selector: "groups_list" } }
-            - can_admin_member = can?(current_user, :admin_group_member, @group)
-            - @group.shared_with_group_links.each do |group_link|
-              = render 'shared/members/group', group_link: group_link, can_admin_member: can_admin_member, group_link_path: group_group_link_path(@group, group_link)
-      .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")
+        = render 'groups/group_members/tab_pane/header' do
+          = render 'groups/group_members/tab_pane/title' do
+            = html_escape(_('Members with access to %{strong_start}%{group_name}%{strong_end}')) % { group_name: @group.name, strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe }
+          = form_tag group_group_members_path(@group), method: :get, class: 'user-search-form gl-display-flex gl-md-align-items-center gl-flex-wrap gl-flex-direction-column gl-md-flex-direction-row gl-mx-n3 gl-my-n3', data: { testid: 'user-search-form' } do
+            .gl-px-3.gl-py-2
+              .search-control-wrap.gl-relative
+                = render 'shared/members/search_field'
             - if can_manage_members
-                = label_tag '2fa', '2FA', class: 'col-form-label label-bold pr-md-2'
+              = render 'groups/group_members/tab_pane/form_item' do
+                = label_tag '2fa', _('2FA'), class: form_item_label_css_class
                 = render 'shared/members/filter_2fa_dropdown'
+            = render 'groups/group_members/tab_pane/form_item' do
+              = label_tag :sort_by, _('Sort by'), class: form_item_label_css_class
               = render 'shared/members/sort_dropdown'
-        %ul.content-list.members-list{ data: { qa_selector: "members_list" } }
+        %ul.content-list.members-list{ data: { qa_selector: 'members_list' } }
           = render partial: 'shared/members/member', collection: @members, as: :member
-      = paginate @members, theme: 'gitlab'
-
+      = paginate @members, theme: 'gitlab', params: { invited_members_page: nil, search_invited: nil }
+    - if @group.shared_with_group_links.any?
+      #tab-groups.tab-pane
+        .card.card-without-border
+          = render 'groups/group_members/tab_pane/header' do
+            = render 'groups/group_members/tab_pane/title' do
+              = html_escape(_('Groups with access to %{strong_start}%{group_name}%{strong_end}')) % { group_name: @group.name, strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe }
+          %ul.content-list.members-list{ data: { qa_selector: 'groups_list' } }
+            - @group.shared_with_group_links.each do |group_link|
+              = render 'shared/members/group', group_link: group_link, can_admin_member: can_manage_members, group_link_path: group_group_link_path(@group, group_link)
     - if show_invited_members
-      #invited_members.tab-pane{ :class => ("active" if pending_active) }
+      #tab-invited-members.tab-pane{ class: ('active' if invited_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")
+          = render 'groups/group_members/tab_pane/header' do
+            = render 'groups/group_members/tab_pane/title' do
+              = html_escape(_('Members invited to %{strong_start}%{group_name}%{strong_end}')) % { group_name: @group.name, strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe }
+            = form_tag group_group_members_path(@group), method: :get, class: 'user-search-form', data: { testid: 'user-search-form' } do
+              = render 'shared/members/search_field', name: 'search_invited'
           %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'
+        = paginate @invited_members, param_name: 'invited_members_page', theme: 'gitlab', params: { page: nil }
+    - if show_access_requests
+      #tab-access-requests.tab-pane
+        .card.card-without-border
+          = render 'groups/group_members/tab_pane/header' do
+            = render 'groups/group_members/tab_pane/title' do
+              = html_escape(_('Users requesting access to %{strong_start}%{group_name}%{strong_end}')) % { group_name: @group.name, strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe }
+          %ul.content-list.members-list
+            = render partial: 'shared/members/member', collection: @requesters, as: :member

app/views/groups/group_members/tab_pane/_form_item.html.haml

+.gl-px-3.gl-py-3.gl-display-flex.gl-flex-direction-column.gl-md-flex-direction-row
+  = yield

app/views/groups/group_members/tab_pane/_header.html.haml

+.gl-display-flex.gl-md-align-items-center.gl-flex-direction-column.gl-md-flex-direction-row.row-content-block.second-block
+  = yield

app/views/groups/group_members/tab_pane/_title.html.haml

+%span.gl-flex-grow-1.gl-py-3.gl-pr-3
+  = yield

app/views/projects/project_members/_team.html.haml

@@ -12,6 +12,7 @@
           = 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") }
             = icon("search")
+        = label_tag :sort_by, _('Sort by'), class: 'col-form-label label-bold px-2'
         = render 'shared/members/sort_dropdown'
   %ul.content-list.members-list{ data: { qa_selector: 'members_list' } }
     = render partial: 'shared/members/member', collection: members, as: :member

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.pr-md-2
+.dropdown.inline.member-filter-2fa-dropdown
   = dropdown_toggle(filter_options[filter], { toggle: 'dropdown' })
   %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-selectable
     %li.dropdown-header

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

+- name = local_assigns.fetch(:name, :search)
+
+.search-control-wrap.gl-relative
+  = search_field_tag name, params[name], { placeholder: _('Search'), class: 'form-control', spellcheck: false }
+  %button.user-search-btn.border-left.gl-display-flex.gl-align-items-center.gl-justify-content-center{ type: 'submit', 'aria': { label: _('Submit search') } }
+    = sprite_icon('search')

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

-= label_tag :sort_by, 'Sort by', class: 'col-form-label label-bold px-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

changelogs/unreleased/229830-move-groups-members-pending-in-to-tabs.yml

+---
+title: Reorganize group member management into tabs
+merge_request: 38344
+author:
+type: changed

locale/gitlab.pot

@@ -1237,6 +1237,9 @@ msgstr ""
 msgid "Access forbidden. Check your access level."
 msgstr ""
 
+msgid "Access requests"
+msgstr ""
+
 msgid "Access to '%{classification_label}' not allowed"
 msgstr ""
 
@@ -9926,9 +9929,6 @@ msgstr ""
 msgid "Existing projects will be able to use expiration policies. Avoid enabling this if an external Container Registry is being used, as there is a performance risk if many images exist on one project."
 msgstr ""
 
-msgid "Existing shares"
-msgstr ""
-
 msgid "Existing sign in methods may be removed"
 msgstr ""
 
@@ -13264,6 +13264,9 @@ msgstr ""
 msgid "Invite teammates (optional)"
 msgstr ""
 
+msgid "Invited"
+msgstr ""
+
 msgid "Invited users will be added with developer level permissions. You can always change this later."
 msgstr ""
 
@@ -14864,6 +14867,9 @@ msgstr ""
 msgid "Members can be added by project %{i_open}Maintainers%{i_close} or %{i_open}Owners%{i_close}"
 msgstr ""
 
+msgid "Members invited to %{strong_start}%{group_name}%{strong_end}"
+msgstr ""
+
 msgid "Members of %{strong_open}%{project_name}%{strong_close}"
 msgstr ""
 
@@ -14873,9 +14879,6 @@ 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 "Memory Usage"
 msgstr ""
 
@@ -26700,6 +26703,9 @@ msgstr ""
 msgid "Users requesting access to"
 msgstr ""
 
+msgid "Users requesting access to %{strong_start}%{group_name}%{strong_end}"
+msgstr ""
+
 msgid "Users were successfully added."
 msgstr ""
 

qa/qa/page/group/members.rb

@@ -25,6 +25,7 @@ module QA
 
         view 'app/views/groups/group_members/index.html.haml' do
           element :invite_group_tab
+          element :groups_list_tab
           element :groups_list
         end
 
@@ -71,6 +72,8 @@ module QA
         end
 
         def has_existing_group_share?(group_name)
+          click_element :groups_list_tab
+
           within_element(:groups_list) do
             has_element?(:group_row, text: group_name)
           end

spec/features/groups/members/manage_groups_spec.rb

@@ -20,26 +20,28 @@ RSpec.describe 'Groups > Members > Manage groups', :js do
 
     add_group(shared_with_group.id, 'Reporter')
 
+    click_groups_tab
+
     page.within(first_row) do
       expect(page).to have_content(shared_with_group.name)
       expect(page).to have_content('Reporter')
     end
   end
 
-  it 'remove user from group' do
+  it 'remove group from group' do
     create(:group_group_link, shared_group: shared_group,
       shared_with_group: shared_with_group, group_access: ::Gitlab::Access::DEVELOPER)
 
     visit group_group_members_path(shared_group)
 
+    click_groups_tab
+
     expect(page).to have_content(shared_with_group.name)
 
     accept_confirm do
-      find(:css, '#existing_shares li', text: shared_with_group.name).find(:css, 'a.btn-remove').click
+      find(:css, '#tab-groups li', text: shared_with_group.name).find(:css, 'a.btn-remove').click
     end
 
-    wait_for_requests
-
     expect(page).not_to have_content(shared_with_group.name)
   end
 
@@ -49,6 +51,8 @@ RSpec.describe 'Groups > Members > Manage groups', :js do
 
     visit group_group_members_path(shared_group)
 
+    click_groups_tab
+
     page.within(first_row) do
       click_button('Developer')
       click_link('Maintainer')
@@ -67,4 +71,8 @@ RSpec.describe 'Groups > Members > Manage groups', :js do
       click_button "Invite"
     end
   end
+
+  def click_groups_tab
+    click_link "Groups"
+  end
 end

spec/features/groups/members/manage_members_spec.rb

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

spec/features/groups/members/master_manages_access_requests_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe 'Groups > Members > Maintainer manages access requests' do
   it_behaves_like 'Maintainer manages access requests' do
+    let(:has_tabs) { true }
     let(:entity) { create(:group, :public) }
     let(:members_page_path) { group_group_members_path(entity) }
   end

spec/features/groups/members/search_members_spec.rb

@@ -19,7 +19,7 @@ RSpec.describe 'Search group member' do
   end
 
   it 'renders member users' do
-    page.within '.user-search-form' do
+    page.within '[data-testid="user-search-form"]' do
       fill_in 'search', with: member.name
       find('.user-search-btn').click
     end

spec/features/groups/members/tabs_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Groups > Members > Tabs' do
+  using RSpec::Parameterized::TableSyntax
+
+  shared_examples 'active "Members" tab' do
+    it 'displays "Members" tab' do
+      expect(page).to have_selector('.nav-link.active', text: 'Members')
+    end
+  end
+
+  shared_examples 'active "Invited" tab' do
+    it 'displays "Invited" tab' do
+      expect(page).to have_selector('.nav-link.active', text: 'Invited')
+    end
+  end
+
+  let(:owner) { create(:user) }
+  let(:group) { create(:group) }
+
+  before do
+    stub_const('Groups::GroupMembersController::MEMBER_PER_PAGE_LIMIT', 1)
+    allow_any_instance_of(Member).to receive(:send_request).and_return(true)
+
+    group.add_owner(owner)
+    sign_in(owner)
+
+    create_list(:group_member, 2, group: group)
+    create_list(:group_member, 2, :invited, group: group)
+    create_list(:group_group_link, 2, shared_group: group)
+    create_list(:group_member, 2, :access_request, group: group)
+  end
+
+  where(:tab, :count) do
+    'Members'         | 3
+    'Invited'         | 2
+    'Groups'          | 2
+    'Access requests' | 2
+  end
+
+  with_them do
+    it "renders #{params[:tab]} tab" do
+      visit group_group_members_path(group)
+
+      expect(page).to have_selector('.nav-link', text: "#{tab} #{count}")
+    end
+  end
+
+  context 'displays "Members" tab by default' do
+    before do
+      visit group_group_members_path(group)
+    end
+
+    it_behaves_like 'active "Members" tab'
+  end
+
+  context 'when searching "Invited"', :js do
+    before do
+      visit group_group_members_path(group)
+
+      click_link 'Invited'
+
+      page.within '[data-testid="user-search-form"]' do
+        fill_in 'search_invited', with: 'email'
+        find('button[type="submit"]').click
+      end
+    end
+
+    it_behaves_like 'active "Invited" tab'
+
+    context 'and then searching "Members"' do
+      before do
+        click_link 'Members'
+
+        page.within '[data-testid="user-search-form"]' do
+          fill_in 'search', with: 'test'
+          find('button[type="submit"]').click
+        end
+      end
+
+      it_behaves_like 'active "Members" tab'
+    end
+  end
+
+  context 'when using "Invited" pagination', :js do
+    before do
+      visit group_group_members_path(group)
+
+      click_link 'Invited'
+
+      page.within '.pagination' do
+        click_link '2'
+      end
+    end
+
+    it_behaves_like 'active "Invited" tab'
+
+    context 'and then using "Members" pagination' do
+      before do
+        click_link 'Members'
+
+        page.within '.pagination' do
+          click_link '2'
+        end
+      end
+
+      it_behaves_like 'active "Members" tab'
+    end
+  end
+end

spec/features/projects/members/master_manages_access_requests_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe 'Projects > Members > Maintainer manages access requests' do
   it_behaves_like 'Maintainer manages access requests' do
+    let(:has_tabs) { false }
     let(:entity) { create(:project, :public) }
     let(:members_page_path) { project_project_members_path(entity) }
   end

spec/support/shared_examples/features/master_manages_access_requests_shared_example.rb

@@ -8,17 +8,18 @@ RSpec.shared_examples 'Maintainer manages access requests' do
     entity.request_access(user)
     entity.respond_to?(:add_owner) ? entity.add_owner(maintainer) : entity.add_maintainer(maintainer)
     sign_in(maintainer)
-  end
-
-  it 'maintainer can see access requests' do
     visit members_page_path
 
+    if has_tabs
+      click_on 'Access requests'
+    end
+  end
+
+  it 'maintainer can see access requests', :js do
     expect_visible_access_request(entity, user)
   end
 
   it 'maintainer can grant access', :js do
-    visit members_page_path
-
     expect_visible_access_request(entity, user)
 
     click_on 'Grant access'
@@ -31,8 +32,6 @@ RSpec.shared_examples 'Maintainer manages access requests' do
   end
 
   it 'maintainer can deny access', :js do
-    visit members_page_path
-
     expect_visible_access_request(entity, user)
 
     # Open modal
@@ -47,7 +46,13 @@ RSpec.shared_examples 'Maintainer manages access requests' do
   end
 
   def expect_visible_access_request(entity, user)
+    if has_tabs
+      expect(page).to have_content "Access requests 1"
+      expect(page).to have_content "Users requesting access to #{entity.name}"
+    else
       expect(page).to have_content "Users requesting access to #{entity.name} 1"
+    end
+
     expect(page).to have_content user.name
   end