Merge branch 'change_invite_banner_button_to_open_modal' into 'master'

Replace invite banner button with modal trigger

See merge request gitlab-org/gitlab!59260

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

 <script>
 import { GlBanner } from '@gitlab/ui';
+import eventHub from '~/invite_members/event_hub';
 import { parseBoolean, setCookie, getCookie } from '~/lib/utils/common_utils';
 import { s__ } from '~/locale';
 import Tracking from '~/tracking';
@@ -11,7 +12,7 @@ export default {
     GlBanner,
   },
   mixins: [trackingMixin],
-  inject: ['svgPath', 'inviteMembersPath', 'isDismissedKey', 'trackLabel'],
+  inject: ['svgPath', 'isDismissedKey', 'trackLabel'],
   data() {
     return {
       isDismissed: parseBoolean(getCookie(this.isDismissedKey)),
@@ -20,11 +21,6 @@ export default {
       },
     };
   },
-  created() {
-    this.$nextTick(() => {
-      this.addTrackingAttributesToButton();
-    });
-  },
   mounted() {
     this.trackOnShow();
   },
@@ -39,15 +35,12 @@ export default {
         if (!this.isDismissed) this.track(this.$options.displayEvent);
       });
     },
-    addTrackingAttributesToButton() {
-      if (this.$refs.banner === undefined) return;
-
-      const button = this.$refs.banner.$el.querySelector(`[href='${this.inviteMembersPath}']`);
-
-      if (button) {
-        button.setAttribute('data-track-event', this.$options.buttonClickEvent);
-        button.setAttribute('data-track-label', this.trackLabel);
-      }
+    openModal() {
+      eventHub.$emit('openModal', {
+        inviteeType: 'members',
+        source: this.$options.openModalSource,
+      });
+      this.track(this.$options.buttonClickEvent);
     },
   },
   i18n: {
@@ -59,6 +52,7 @@ export default {
   },
   displayEvent: 'invite_members_banner_displayed',
   buttonClickEvent: 'invite_members_banner_button_clicked',
+  openModalSource: 'invite_members_banner',
   dismissEvent: 'invite_members_banner_dismissed',
 };
 </script>
@@ -70,8 +64,8 @@ export default {
     :title="$options.i18n.title"
     :button-text="$options.i18n.button_text"
     :svg-path="svgPath"
-    :button-link="inviteMembersPath"
     @close="handleClose"
+    @primary="openModal"
   >
     <p>{{ $options.i18n.body }}</p>
   </gl-banner>

app/assets/javascripts/pages/groups/shared/group_details.js

@@ -3,6 +3,7 @@
 import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation';
 import { ACTIVE_TAB_SHARED, ACTIVE_TAB_ARCHIVED } from '~/groups/constants';
 import initInviteMembersBanner from '~/groups/init_invite_members_banner';
+import initInviteMembersModal from '~/invite_members/init_invite_members_modal';
 import { getPagePath, getDashPath } from '~/lib/utils/common_utils';
 import initNotificationsDropdown from '~/notifications';
 import ProjectsList from '~/projects_list';
@@ -24,4 +25,5 @@ export default function initGroupDetails(actionName = 'show') {
   new ProjectsList();
 
   initInviteMembersBanner();
+  initInviteMembersModal();
 }

app/helpers/invite_members_helper.rb

@@ -3,10 +3,6 @@
 module InviteMembersHelper
   include Gitlab::Utils::StrongMemoize
 
-  def can_invite_members_for_group?(group)
-    Feature.enabled?(:invite_members_group_modal, group) && can?(current_user, :admin_group_member, group)
-  end
-
   def can_invite_members_for_project?(project)
     Feature.enabled?(:invite_members_group_modal, project.group) && can_manage_project_members?(project)
   end

app/views/groups/_invite_members_modal.html.haml

-- if can_invite_members_for_group?(group)
+- if can?(current_user, :admin_group_member, group)
   .js-invite-members-modal{ data: { id: group.id,
     name: group.name,
     is_project: 'false',

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

@@ -15,13 +15,13 @@
             = _('Group members')
           %p
             = html_escape(_('You can invite a new member to %{strong_start}%{group_name}%{strong_end}.')) % { group_name: @group.name, strong_start: '<strong>'.html_safe, strong_end: '</strong>'.html_safe }
-        - if can_invite_members_for_group?(@group)
+        - if Feature.enabled?(:invite_members_group_modal, @group)
           .gl-w-half.gl-xs-w-full
             .gl-display-flex.gl-flex-wrap.gl-justify-content-end.gl-mb-3
               .js-invite-group-trigger{ data: { classes: 'gl-mt-3 gl-sm-w-auto gl-w-full', display_text: _('Invite a group') } }
               .js-invite-members-trigger{ data: { variant: 'success', classes: 'gl-mt-3 gl-sm-w-auto gl-w-full gl-sm-ml-3', display_text: _('Invite members') } }
           = render 'groups/invite_members_modal', group: @group
-    - if can_manage_members && !can_invite_members_for_group?(@group)
+    - if can_manage_members && Feature.disabled?(:invite_members_group_modal, @group)
       %hr.gl-mt-4
         %ul.nav-links.nav.nav-tabs.gitlab-tabs{ role: 'tablist' }
           %li.nav-tab{ role: 'presentation' }

app/views/groups/show.html.haml

@@ -12,6 +12,7 @@
         is_dismissed_key: "invite_#{@group.id}_#{current_user.id}",
         track_label: 'invite_members_banner',
         invite_members_path: group_group_members_path(@group) } }
+  = render 'groups/invite_members_modal', group: @group
 
 = content_for :meta_tags do
   = auto_discovery_link_tag(:atom, group_url(@group, rss_url_options), title: "#{@group.name} activity")

changelogs/unreleased/change_invite_banner_button_to_open_modal.yml

+---
+title: Replace invite banner button with modal trigger
+merge_request: 59260
+author:
+type: changed

spec/features/groups/members/manage_members_spec.rb

@@ -26,6 +26,18 @@ RSpec.describe 'Groups > Members > Manage members' do
     end
   end
 
+  shared_examples 'does not include either invite modal or either invite form' do
+    it 'does not include either of the invite members or invite group modal buttons' do
+      expect(page).not_to have_selector '.js-invite-members-modal'
+      expect(page).not_to have_selector '.js-invite-group-modal'
+    end
+
+    it 'does not include either of the invite users or invite group forms' do
+      expect(page).not_to have_selector '.invite-users-form'
+      expect(page).not_to have_selector '.invite-group-form'
+    end
+  end
+
   context 'when Invite Members modal is enabled' do
     it_behaves_like 'includes the correct Invite link', '.js-invite-members-trigger', '.invite-users-form'
     it_behaves_like 'includes the correct Invite link', '.js-invite-group-trigger', '.invite-group-form'
@@ -165,23 +177,46 @@ RSpec.describe 'Groups > Members > Manage members' do
     end
   end
 
-  it 'guest can not manage other users', :js do
-    group.add_guest(user1)
-    group.add_developer(user2)
+  context 'as a guest', :js do
+    before do
+      group.add_guest(user1)
+      group.add_developer(user2)
 
-    visit group_group_members_path(group)
+      visit group_group_members_path(group)
+    end
 
-    expect(page).not_to have_selector '.invite-users-form'
-    expect(page).not_to have_selector '.invite-group-form'
-    expect(page).not_to have_selector '.js-invite-members-modal'
-    expect(page).not_to have_selector '.js-invite-group-modal'
+    it_behaves_like 'does not include either invite modal or either invite form'
 
-    page.within(second_row) do
-      # Can not modify user2 role
-      expect(page).not_to have_button 'Developer'
+    it 'does not include a button on the members page list to manage or remove the existing member', :js do
+      page.within(second_row) do
+        # Can not modify user2 role
+        expect(page).not_to have_button 'Developer'
+
+        # Can not remove user2
+        expect(page).not_to have_selector 'button[title="Remove member"]'
+      end
+    end
+  end
+
+  context 'As a guest when the :invite_members_group_modal feature flag is disabled', :js do
+    before do
+      stub_feature_flags(invite_members_group_modal: false)
+      group.add_guest(user1)
+      group.add_developer(user2)
+
+      visit group_group_members_path(group)
+    end
+
+    it_behaves_like 'does not include either invite modal or either invite form'
+
+    it 'does not include a button on the members page list to manage or remove the existing member', :js do
+      page.within(second_row) do
+        # Can not modify user2 role
+        expect(page).not_to have_button 'Developer'
 
-      # Can not remove user2
-      expect(page).not_to have_selector 'button[title="Remove member"]'
+        # Can not remove user2
+        expect(page).not_to have_selector 'button[title="Remove member"]'
+      end
     end
   end
 end

spec/frontend/groups/components/invite_members_banner_spec.js

@@ -2,6 +2,7 @@ import { GlBanner, GlButton } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
 import { mockTracking, unmockTracking } from 'helpers/tracking_helper';
 import InviteMembersBanner from '~/groups/components/invite_members_banner.vue';
+import eventHub from '~/invite_members/event_hub';
 import { setCookie, parseBoolean } from '~/lib/utils/common_utils';
 
 jest.mock('~/lib/utils/common_utils');
@@ -58,12 +59,23 @@ describe('InviteMembersBanner', () => {
       });
     });
 
-    it('sets the button attributes for the buttonClickEvent', () => {
-      const button = wrapper.find(`[href='${wrapper.vm.inviteMembersPath}']`);
+    describe('when the button is clicked', () => {
+      beforeEach(() => {
+        jest.spyOn(eventHub, '$emit').mockImplementation(() => {});
+        wrapper.find(GlBanner).vm.$emit('primary');
+      });
+
+      it('calls openModal through the eventHub', () => {
+        expect(eventHub.$emit).toHaveBeenCalledWith('openModal', {
+          inviteeType: 'members',
+          source: 'invite_members_banner',
+        });
+      });
 
-      expect(button.attributes()).toMatchObject({
-        'data-track-event': buttonClickEvent,
-        'data-track-label': trackLabel,
+      it('sends the buttonClickEvent with correct trackCategory and trackLabel', () => {
+        expect(trackingSpy).toHaveBeenCalledWith(trackCategory, buttonClickEvent, {
+          label: trackLabel,
+        });
       });
     });
 
@@ -100,10 +112,6 @@ describe('InviteMembersBanner', () => {
     it('uses the button_text text from options for buttontext', () => {
       expect(findBanner().attributes('buttontext')).toBe(buttonText);
     });
-
-    it('uses the href from inviteMembersPath for buttonlink', () => {
-      expect(findBanner().attributes('buttonlink')).toBe(inviteMembersPath);
-    });
   });
 
   describe('dismissing', () => {

spec/helpers/invite_members_helper_spec.rb

@@ -80,49 +80,6 @@ RSpec.describe InviteMembersHelper do
   context 'with group' do
     let_it_be(:group) { create(:group) }
 
-    describe "#can_invite_members_for_group?" do
-      include Devise::Test::ControllerHelpers
-
-      let_it_be(:user) { create(:user) }
-
-      before do
-        sign_in(user)
-        allow(helper).to receive(:current_user) { user }
-      end
-
-      context 'when the user can admin_group_member' do
-        before do
-          allow(helper).to receive(:can?).with(user, :admin_group_member, group).and_return(true)
-        end
-
-        it 'returns true' do
-          expect(helper.can_invite_members_for_group?(group)).to eq true
-          expect(helper).to have_received(:can?).with(user, :admin_group_member, group)
-        end
-
-        context 'when feature flag is disabled' do
-          before do
-            stub_feature_flags(invite_members_group_modal: false)
-          end
-
-          it 'returns false' do
-            expect(helper.can_invite_members_for_group?(group)).to eq false
-            expect(helper).not_to have_received(:can?)
-          end
-        end
-      end
-
-      context 'when the user can not admin_group_member' do
-        before do
-          expect(helper).to receive(:can?).with(user, :admin_group_member, group).and_return(false)
-        end
-
-        it 'returns false' do
-          expect(helper.can_invite_members_for_group?(group)).to eq false
-        end
-      end
-    end
-
     describe "#invite_group_members?" do
       context 'when the user is an owner' do
         before do

spec/views/groups/show.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'groups/edit.html.haml' do
+  include Devise::Test::ControllerHelpers
+
+  describe '"Share with group lock" setting' do
+    let(:root_owner) { create(:user) }
+    let(:root_group) { create(:group) }
+
+    before do
+      root_group.add_owner(root_owner)
+    end
+
+    shared_examples_for '"Share with group lock" setting' do |checkbox_options|
+      it 'has the correct label, help text, and checkbox options' do
+        assign(:group, test_group)
+        allow(view).to receive(:can?).with(test_user, :admin_group, test_group).and_return(true)
+        allow(view).to receive(:can_change_group_visibility_level?).and_return(false)
+        allow(view).to receive(:current_user).and_return(test_user)
+        expect(view).to receive(:can_change_share_with_group_lock?).and_return(!checkbox_options[:disabled])
+        expect(view).to receive(:share_with_group_lock_help_text).and_return('help text here')
+
+        render
+
+        expect(rendered).to have_content("Prevent sharing a project within #{test_group.name} with other groups")
+        expect(rendered).to have_css('.js-descr', text: 'help text here')
+        expect(rendered).to have_field('group_share_with_group_lock', **checkbox_options)
+      end
+    end
+
+    context 'for a root group' do
+      let(:test_group) { root_group }
+      let(:test_user) { root_owner }
+
+      it_behaves_like '"Share with group lock" setting', { disabled: false, checked: false }
+    end
+
+    context 'for a subgroup' do
+      let!(:subgroup) { create(:group, parent: root_group) }
+      let(:sub_owner) { create(:user) }
+      let(:test_group) { subgroup }
+
+      context 'when the root_group has "Share with group lock" disabled' do
+        context 'when the subgroup has "Share with group lock" disabled' do
+          context 'as the root_owner' do
+            let(:test_user) { root_owner }
+
+            it_behaves_like '"Share with group lock" setting', { disabled: false, checked: false }
+          end
+
+          context 'as the sub_owner' do
+            let(:test_user) { sub_owner }
+
+            it_behaves_like '"Share with group lock" setting', { disabled: false, checked: false }
+          end
+        end
+
+        context 'when the subgroup has "Share with group lock" enabled' do
+          before do
+            subgroup.update_column(:share_with_group_lock, true)
+          end
+
+          context 'as the root_owner' do
+            let(:test_user) { root_owner }
+
+            it_behaves_like '"Share with group lock" setting', { disabled: false, checked: true }
+          end
+
+          context 'as the sub_owner' do
+            let(:test_user) { sub_owner }
+
+            it_behaves_like '"Share with group lock" setting', { disabled: false, checked: true }
+          end
+        end
+      end
+
+      context 'when the root_group has "Share with group lock" enabled' do
+        before do
+          root_group.update_column(:share_with_group_lock, true)
+        end
+
+        context 'when the subgroup has "Share with group lock" disabled (parent overridden)' do
+          context 'as the root_owner' do
+            let(:test_user) { root_owner }
+
+            it_behaves_like '"Share with group lock" setting', { disabled: false, checked: false }
+          end
+
+          context 'as the sub_owner' do
+            let(:test_user) { sub_owner }
+
+            it_behaves_like '"Share with group lock" setting', { disabled: false, checked: false }
+          end
+        end
+
+        context 'when the subgroup has "Share with group lock" enabled (same as parent)' do
+          before do
+            subgroup.update_column(:share_with_group_lock, true)
+          end
+
+          context 'as the root_owner' do
+            let(:test_user) { root_owner }
+
+            it_behaves_like '"Share with group lock" setting', { disabled: false, checked: true }
+          end
+
+          context 'as the sub_owner' do
+            let(:test_user) { sub_owner }
+
+            it_behaves_like '"Share with group lock" setting', { disabled: true, checked: true }
+          end
+        end
+      end
+    end
+  end
+end