Merge branch '328519-do-not-load-cohorts-unless-necessary' into 'master'

Prevent loading of cohorts data if not necessary

See merge request gitlab-org/gitlab!59890

app/assets/javascripts/admin/users/tabs.js

-import Api from '~/api';
-import { historyPushState } from '~/lib/utils/common_utils';
-import { mergeUrlParams } from '~/lib/utils/url_utility';
-
-const COHORTS_PANE = 'cohorts';
-const COHORTS_PANE_TAB_CLICK_EVENT = 'i_analytics_cohorts';
-
-const tabClickHandler = (e) => {
-  const { hash } = e.currentTarget;
-
-  let tab = null;
-
-  if (hash === `#${COHORTS_PANE}`) {
-    tab = COHORTS_PANE;
-    Api.trackRedisHllUserEvent(COHORTS_PANE_TAB_CLICK_EVENT);
-  }
-
-  const newUrl = mergeUrlParams({ tab }, window.location.href);
-  historyPushState(newUrl);
-};
-
-const initTabs = () => {
-  const tabLinks = document.querySelectorAll('.js-users-tab-item a');
-
-  if (tabLinks.length) {
-    tabLinks.forEach((tabLink) => {
-      tabLink.addEventListener('click', (e) => tabClickHandler(e));
-    });
-  }
-};
-
-export default initTabs;

app/assets/javascripts/pages/admin/users/index.js

 import Vue from 'vue';
 
 import { initAdminUsersApp } from '~/admin/users';
-import initTabs from '~/admin/users/tabs';
 import initConfirmModal from '~/confirm_modal';
 import csrf from '~/lib/utils/csrf';
 import Translate from '~/vue_shared/translate';
@@ -62,5 +61,4 @@ document.addEventListener('DOMContentLoaded', () => {
   });
 
   initConfirmModal();
-  initTabs();
 });

app/controllers/admin/cohorts_controller.rb

@@ -6,6 +6,6 @@ class Admin::CohortsController < Admin::ApplicationController
   # Backwards compatibility. Remove it and routing in 14.0
   # @see https://gitlab.com/gitlab-org/gitlab/-/issues/299303
   def index
-    redirect_to admin_users_path(tab: 'cohorts')
+    redirect_to cohorts_admin_users_path
   end
 end

app/controllers/admin/users_controller.rb

@@ -4,7 +4,7 @@ class Admin::UsersController < Admin::ApplicationController
   include RoutableActions
   include Analytics::UniqueVisitsHelper
 
-  before_action :user, except: [:index, :new, :create]
+  before_action :user, except: [:index, :cohorts, :new, :create]
   before_action :check_impersonation_availability, only: :impersonate
   before_action :ensure_destroy_prerequisites_met, only: [:destroy]
 
@@ -13,16 +13,19 @@ class Admin::UsersController < Admin::ApplicationController
   PAGINATION_WITH_COUNT_LIMIT = 1000
 
   def index
+    return redirect_to cohorts_admin_users_path if params[:tab] == 'cohorts'
+
     @users = User.filter_items(params[:filter]).order_name_asc
     @users = @users.search_with_secondary_emails(params[:search_query]) if params[:search_query].present?
     @users = users_with_included_associations(@users)
     @users = @users.sort_by_attribute(@sort = params[:sort])
     @users = @users.page(params[:page])
     @users = @users.without_count if paginate_without_count?
+  end
 
+  def cohorts
     @cohorts = load_cohorts
-
-    track_cohorts_visit if params[:tab] == 'cohorts'
+    track_cohorts_visit
   end
 
   def show

app/views/admin/users/_tabs.html.haml

+%ul.nav-links.nav-tabs.nav.js-users-tabs{ role: 'tablist' }
+  %li.nav-item{ role: 'presentation' }
+    %a.nav-link{ href: admin_users_path, class: active_when(current_page?(admin_users_path)), role: 'tab' }
+      = s_('AdminUsers|Users')
+  %li.nav-item{ role: 'presentation' }
+    %a.nav-link{ href: cohorts_admin_users_path, class: active_when(current_page?(cohorts_admin_users_path)), role: 'tab' }
+      = s_('AdminUsers|Cohorts')

app/views/admin/users/cohorts.html.haml

+- page_title _("Users")
+
+= render 'tabs'
+
+.tab-content
+  .tab-pane.active
+    = render 'cohorts'

app/views/admin/users/index.html.haml

 - page_title _("Users")
 
-%ul.nav-links.nav-tabs.nav.js-users-tabs{ role: 'tablist' }
-  %li.nav-item.js-users-tab-item{ role: 'presentation' }
-    %a.nav-link{ href: '#users', class: active_when(params[:tab] != 'cohorts'), data: { toggle: 'tab' }, role: 'tab' }
-      = s_('AdminUsers|Users')
-  %li.nav-item.js-users-tab-item{ role: 'presentation' }
-    %a.nav-link{ href: '#cohorts', class: active_when(params[:tab] == 'cohorts'), data: { toggle: 'tab' }, role: 'tab' }
-      = s_('AdminUsers|Cohorts')
+= render 'tabs'
 
 .tab-content
-  .tab-pane{ id: 'users', class: ('active' if params[:tab] != 'cohorts') }
+  .tab-pane.active
     = render 'users'
-  .tab-pane{ id: 'cohorts', class: ('active' if params[:tab] == 'cohorts') }
-    = render 'cohorts'
-
-

changelogs/unreleased/328519-do-not-load-cohorts-unless-necessary.yml

+---
+title: Prevent loading of cohorts data in the admin users list
+merge_request: 59890
+author:
+type: performance

config/routes/admin.rb

@@ -10,6 +10,12 @@ namespace :admin do
       end
     end
 
+    collection do
+      scope '/-/' do
+        get :cohorts
+      end
+    end
+
     member do
       get :projects
       get :keys

spec/controllers/admin/cohorts_controller_spec.rb

@@ -12,6 +12,6 @@ RSpec.describe Admin::CohortsController do
   it 'redirects to Overview->Users' do
     get :index
 
-    expect(response).to redirect_to(admin_users_path(tab: 'cohorts'))
+    expect(response).to redirect_to(cohorts_admin_users_path)
   end
 end

spec/controllers/admin/users_controller_spec.rb

@@ -30,11 +30,6 @@ RSpec.describe Admin::UsersController do
       expect(assigns(:users).first.association(:authorized_projects)).to be_loaded
     end
 
-    it_behaves_like 'tracking unique visits', :index do
-      let(:target_id) { 'i_analytics_cohorts' }
-      let(:request_params) { { tab: 'cohorts' } }
-    end
-
     context 'pagination' do
       context 'when number of users is over the pagination limit' do
         before do
@@ -59,6 +54,12 @@ RSpec.describe Admin::UsersController do
     end
   end
 
+  describe 'GET #cohorts' do
+    it_behaves_like 'tracking unique visits', :cohorts do
+      let(:target_id) { 'i_analytics_cohorts' }
+    end
+  end
+
   describe 'GET :id' do
     it 'finds a user case-insensitively' do
       user = create(:user, username: 'CaseSensitive')

spec/features/admin/admin_users_spec.rb

@@ -10,36 +10,39 @@ RSpec.describe "Admin::Users" do
     gitlab_enable_admin_mode_sign_in(current_user)
   end
 
-  describe 'Tabs', :js do
+  describe 'Tabs' do
     let(:tabs_selector) { '.js-users-tabs' }
     let(:active_tab_selector) { '.nav-link.active' }
 
-    it 'does not add the tab param when the Users tab is selected' do
-      visit admin_users_path
+    it 'links to the Users tab' do
+      visit cohorts_admin_users_path
 
       within tabs_selector do
         click_link 'Users'
+
+        expect(page).to have_selector active_tab_selector, text: 'Users'
       end
 
       expect(page).to have_current_path(admin_users_path)
     end
 
-    it 'adds the ?tab=cohorts param when the Cohorts tab is selected' do
+    it 'links to the Cohorts tab' do
       visit admin_users_path
 
       within tabs_selector do
         click_link 'Cohorts'
+
+        expect(page).to have_selector active_tab_selector, text: 'Cohorts'
       end
 
-      expect(page).to have_current_path(admin_users_path(tab: 'cohorts'))
+      expect(page).to have_current_path(cohorts_admin_users_path)
+      expect(page).to have_selector active_tab_selector, text: 'Cohorts'
     end
 
-    it 'shows the cohorts tab when the tab param is set' do
+    it 'redirects legacy route' do
       visit admin_users_path(tab: 'cohorts')
 
-      within tabs_selector do
-        expect(page).to have_selector active_tab_selector, text: 'Cohorts'
-      end
+      expect(page).to have_current_path(cohorts_admin_users_path)
     end
   end
 

spec/frontend/admin/users/tabs_spec.js

-import initTabs from '~/admin/users/tabs';
-import Api from '~/api';
-
-jest.mock('~/api.js');
-jest.mock('~/lib/utils/common_utils');
-
-describe('tabs', () => {
-  beforeEach(() => {
-    setFixtures(`
-    <div>
-      <div class="js-users-tab-item">
-        <a href="#users" data-testid='users-tab'>Users</a>
-      </div>
-      <div class="js-users-tab-item">
-        <a href="#cohorts" data-testid='cohorts-tab'>Cohorts</a>
-      </div>
-    </div`);
-
-    initTabs();
-  });
-
-  afterEach(() => {});
-
-  describe('tracking', () => {
-    it('tracks event when cohorts tab is clicked', () => {
-      document.querySelector('[data-testid="cohorts-tab"]').click();
-
-      expect(Api.trackRedisHllUserEvent).toHaveBeenCalledWith('i_analytics_cohorts');
-    });
-
-    it('does not track an event when users tab is clicked', () => {
-      document.querySelector('[data-testid="users-tab"]').click();
-
-      expect(Api.trackRedisHllUserEvent).not.toHaveBeenCalled();
-    });
-  });
-});