Automatic merge of gitlab-org/gitlab master

app/finders/packages/maven/package_finder.rb

@@ -3,13 +3,15 @@
 module Packages
   module Maven
     class PackageFinder
-      attr_reader :path, :current_user, :project, :group
+      include ::Packages::FinderHelper
+      include Gitlab::Utils::StrongMemoize
 
-      def initialize(path, current_user, project: nil, group: nil)
+      def initialize(path, current_user, project: nil, group: nil, order_by_package_file: false)
         @path = path
         @current_user = current_user
         @project = project
         @group = group
+        @order_by_package_file = order_by_package_file
       end
 
       def execute
@@ -23,9 +25,9 @@ module Packages
       private
 
       def base
-        if project
+        if @project
           packages_for_a_single_project
-        elsif group
+        elsif @group
           packages_for_multiple_projects
         else
           ::Packages::Package.none
@@ -33,8 +35,13 @@ module Packages
       end
 
       def packages_with_path
-        matching_packages = base.only_maven_packages_with_path(path, use_cte: group.present?)
-        matching_packages = matching_packages.order_by_package_file if versionless_package?(matching_packages)
+        matching_packages = base.only_maven_packages_with_path(@path, use_cte: @group.present?)
+
+        if group_level_improvements?
+          matching_packages = matching_packages.order_by_package_file if @order_by_package_file
+        else
+          matching_packages = matching_packages.order_by_package_file if versionless_package?(matching_packages)
+        end
 
         matching_packages
       end
@@ -48,19 +55,29 @@ module Packages
 
       # Produces a query that retrieves packages from a single project.
       def packages_for_a_single_project
-        project.packages
+        @project.packages
       end
 
       # Produces a query that retrieves packages from multiple projects that
       # the current user can view within a group.
       def packages_for_multiple_projects
-        ::Packages::Package.for_projects(projects_visible_to_current_user)
+        if group_level_improvements?
+          packages_visible_to_user(@current_user, within_group: @group)
+        else
+          ::Packages::Package.for_projects(projects_visible_to_current_user)
+        end
       end
 
       # Returns the projects that the current user can view within a group.
       def projects_visible_to_current_user
-        group.all_projects
-             .public_or_visible_to_user(current_user)
+        @group.all_projects
+              .public_or_visible_to_user(@current_user)
+      end
+
+      def group_level_improvements?
+        strong_memoize(:group_level_improvements) do
+          Feature.enabled?(:maven_packages_group_level_improvements)
+        end
       end
     end
   end

app/models/packages/package.rb

@@ -136,7 +136,9 @@ class Packages::Package < ApplicationRecord
   after_commit :update_composer_cache, on: :destroy, if: -> { composer? }
 
   def self.for_projects(projects)
-    return none unless projects.any?
+    unless Feature.enabled?(:maven_packages_group_level_improvements)
+      return none unless projects.any?
+    end
 
     where(project_id: projects)
   end

app/models/user.rb

@@ -1851,10 +1851,12 @@ class User < ApplicationRecord
   end
 
   def dismissed_callout?(feature_name:, ignore_dismissal_earlier_than: nil)
-    callouts = self.callouts.with_feature_name(feature_name)
-    callouts = callouts.with_dismissed_after(ignore_dismissal_earlier_than) if ignore_dismissal_earlier_than
+    callout = callouts_by_feature_name[feature_name]
 
-    callouts.any?
+    return false unless callout
+    return callout.dismissed_after?(ignore_dismissal_earlier_than) if ignore_dismissal_earlier_than
+
+    true
   end
 
   # Load the current highest access by looking directly at the user's memberships
@@ -1917,6 +1919,10 @@ class User < ApplicationRecord
 
   private
 
+  def callouts_by_feature_name
+    @callouts_by_feature_name ||= callouts.index_by(&:feature_name)
+  end
+
   def authorized_groups_without_shared_membership
     Group.from_union([
       groups,

app/models/user_callout.rb

@@ -38,6 +38,7 @@ class UserCallout < ApplicationRecord
     uniqueness: { scope: :user_id },
     inclusion: { in: UserCallout.feature_names.keys }
 
-  scope :with_feature_name, -> (feature_name) { where(feature_name: UserCallout.feature_names[feature_name]) }
-  scope :with_dismissed_after, -> (dismissed_after) { where('dismissed_at > ?', dismissed_after) }
+  def dismissed_after?(dismissed_after)
+    dismissed_at > dismissed_after
+  end
 end

app/views/admin/groups/show.html.haml

@@ -126,7 +126,7 @@
     .card
       .card-header
         = html_escape(_("%{group_name} group members")) % { group_name: "<strong>#{html_escape(@group.name)}</strong>".html_safe }
-        %span.badge.badge-pill= @group.members.size
+        %span.badge.badge-pill= @group.users_count
         .float-right
           = link_to group_group_members_path(@group), class: 'btn btn-default gl-button btn-sm' do
             = sprite_icon('pencil-square', css_class: 'gl-icon')

changelogs/unreleased/ci-allow-external-validation-request-timeout-override.yml

+---
+title: Make VALIDATION_REQUEST_TIMEOUT configurable
+merge_request: 57521
+author:
+type: changed

changelogs/unreleased/id-preload-all-user-callouts.yml

+---
+title: Preload all user callouts in a single request
+merge_request: 57679
+author:
+type: performance

config/feature_flags/development/maven_packages_group_level_improvements.yml

+---
+name: maven_packages_group_level_improvements
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57600
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/326099
+milestone: '13.11'
+type: development
+group: group::package
+default_enabled: false

doc/administration/external_pipeline_validation.md

@@ -27,7 +27,15 @@ Response Code Legend:
 
 ## Configuration
 
-Set the `EXTERNAL_VALIDATION_SERVICE_URL` to the external service URL and enable `ci_external_validation_service` feature flag.
+To configure external pipeline validation:
+
+1. Set the `EXTERNAL_VALIDATION_SERVICE_URL` environment variable to the external
+   service URL.
+1. Enable the `ci_external_validation_service` feature flag.
+
+By default, requests to the external service time out after five seconds. To override
+the default, set the `EXTERNAL_VALIDATION_SERVICE_TIMEOUT` environment variable to the
+required number of seconds.
 
 ## Payload Schema
 

ee/app/models/ee/group.rb

@@ -431,7 +431,7 @@ module EE
 
     override :users_count
     def users_count
-      return all_group_members.count unless minimal_access_role_allowed?
+      return all_group_members.count if minimal_access_role_allowed?
 
       members.count
     end

ee/changelogs/unreleased/325729-admin-count-doesn-t-include-minimal-access-users.yml

+---
+title: Count minimal access users in admin area group page
+merge_request: 57630
+author:
+type: fixed

ee/spec/helpers/ee/ci/runners_helper_spec.rb

@@ -3,7 +3,7 @@
 require "spec_helper"
 
 RSpec.describe EE::Ci::RunnersHelper do
-  let_it_be(:user) { create(:user) }
+  let_it_be(:user, refind: true) { create(:user) }
   let_it_be(:namespace) { create(:namespace, owner: user) }
   let_it_be(:project) { create(:project, namespace: namespace) }
 

ee/spec/models/ee/group_spec.rb

@@ -765,6 +765,35 @@ RSpec.describe Group do
     end
   end
 
+  describe '#users_count' do
+    subject { group.users_count }
+
+    let(:group) { create(:group) }
+    let(:user) { create(:user) }
+
+    context 'with `minimal_access_role` not licensed' do
+      before do
+        stub_licensed_features(minimal_access_role: false)
+        create(:group_member, :minimal_access, user: user, source: group)
+      end
+
+      it 'does not count the minimal access user' do
+        expect(group.users_count).to eq(0)
+      end
+    end
+
+    context 'with `minimal_access_role` licensed' do
+      before do
+        stub_licensed_features(minimal_access_role: true)
+        create(:group_member, :minimal_access, user: user, source: group)
+      end
+
+      it 'counts the minimal access user' do
+        expect(group.users_count).to eq(1)
+      end
+    end
+  end
+
   describe '#saml_discovery_token' do
     it 'returns existing tokens' do
       group = create(:group, saml_discovery_token: 'existing')

ee/spec/services/security/store_report_service_spec.rb

@@ -279,7 +279,7 @@ RSpec.describe Security::StoreReportService, '#execute' do
     end
 
     context 'vulnerability issue link' do
-      context 'when there is no assoiciated issue feedback with finding' do
+      context 'when there is no associated issue feedback with finding' do
         it 'does not insert issue links from the new pipeline' do
           expect { subject }.to change { Vulnerabilities::IssueLink.count }.by(0)
         end

lib/api/maven_packages.rb

@@ -77,6 +77,22 @@ module API
           request.head? &&
           file.fog_credentials[:provider] == 'AWS'
       end
+
+      def fetch_package(file_name:, project: nil, group: nil)
+        order_by_package_file = false
+        if Feature.enabled?(:maven_packages_group_level_improvements)
+          order_by_package_file = file_name.include?(::Packages::Maven::Metadata.filename) &&
+                                    !params[:path].include?(::Packages::Maven::FindOrCreatePackageService::SNAPSHOT_TERM)
+        end
+
+        ::Packages::Maven::PackageFinder.new(
+          params[:path],
+          current_user,
+          project: project,
+          group: group,
+          order_by_package_file: order_by_package_file
+        ).execute!
+      end
     end
 
     desc 'Download the maven package file at instance level' do
@@ -97,8 +113,7 @@ module API
 
       authorize_read_package!(project)
 
-      package = ::Packages::Maven::PackageFinder
-        .new(params[:path], current_user, project: project).execute!
+      package = fetch_package(file_name: file_name, project: project)
 
       package_file = ::Packages::PackageFileFinder
         .new(package, file_name).execute!
@@ -133,8 +148,7 @@ module API
 
         not_found!('Group') unless can?(current_user, :read_group, group)
 
-        package = ::Packages::Maven::PackageFinder
-          .new(params[:path], current_user, group: group).execute!
+        package = fetch_package(file_name: file_name, group: group)
 
         authorize_read_package!(package.project)
 
@@ -171,8 +185,7 @@ module API
 
         file_name, format = extract_format(params[:file_name])
 
-        package = ::Packages::Maven::PackageFinder
-          .new(params[:path], current_user, project: user_project).execute!
+        package = fetch_package(file_name: file_name, project: user_project)
 
         package_file = ::Packages::PackageFileFinder
           .new(package, file_name).execute!

lib/gitlab/ci/pipeline/chain/validate/external.rb

@@ -10,7 +10,7 @@ module Gitlab
 
             InvalidResponseCode = Class.new(StandardError)
 
-            VALIDATION_REQUEST_TIMEOUT = 5
+            DEFAULT_VALIDATION_REQUEST_TIMEOUT = 5
             ACCEPTED_STATUS = 200
             DOT_COM_REJECTED_STATUS = 406
             GENERAL_REJECTED_STATUS = (400..499).freeze
@@ -70,11 +70,18 @@ module Gitlab
 
             def validate_service_request
               Gitlab::HTTP.post(
-                validation_service_url, timeout: VALIDATION_REQUEST_TIMEOUT,
+                validation_service_url, timeout: validation_service_timeout,
                 body: validation_service_payload.to_json
               )
             end
 
+            def validation_service_timeout
+              timeout = ENV['EXTERNAL_VALIDATION_SERVICE_TIMEOUT'].to_i
+              return timeout if timeout > 0
+
+              DEFAULT_VALIDATION_REQUEST_TIMEOUT
+            end
+
             def validation_service_url
               ENV['EXTERNAL_VALIDATION_SERVICE_URL']
             end

spec/finders/packages/maven/package_finder_spec.rb

@@ -11,7 +11,8 @@ RSpec.describe ::Packages::Maven::PackageFinder do
   let(:param_path) { nil }
   let(:param_project) { nil }
   let(:param_group) { nil }
-  let(:finder) { described_class.new(param_path, user, project: param_project, group: param_group) }
+  let(:param_order_by_package_file) { false }
+  let(:finder) { described_class.new(param_path, user, project: param_project, group: param_group, order_by_package_file: param_order_by_package_file) }
 
   before do
     group.add_developer(user)
@@ -46,7 +47,23 @@ RSpec.describe ::Packages::Maven::PackageFinder do
       context 'within a group' do
         let(:param_group) { group }
 
-        it_behaves_like 'handling valid and invalid paths'
+        context 'with maven_packages_group_level_improvements enabled' do
+          before do
+            stub_feature_flags(maven_packages_group_level_improvements: true)
+            expect(finder).to receive(:packages_visible_to_user).with(user, within_group: group).and_call_original
+          end
+
+          it_behaves_like 'handling valid and invalid paths'
+        end
+
+        context 'with maven_packages_group_level_improvements disabled' do
+          before do
+            stub_feature_flags(maven_packages_group_level_improvements: false)
+            expect(finder).not_to receive(:packages_visible_to_user)
+          end
+
+          it_behaves_like 'handling valid and invalid paths'
+        end
       end
 
       context 'across all projects' do
@@ -76,7 +93,39 @@ RSpec.describe ::Packages::Maven::PackageFinder do
           create(:package_file, :xml, package: package2)
         end
 
-        it { is_expected.to eq(package2) }
+        context 'with maven_packages_group_level_improvements enabled' do
+          before do
+            stub_feature_flags(maven_packages_group_level_improvements: true)
+            expect(finder).not_to receive(:versionless_package?)
+          end
+
+          context 'without order by package file' do
+            it { is_expected.to eq(package3) }
+          end
+
+          context 'with order by package file' do
+            let(:param_order_by_package_file) { true }
+
+            it { is_expected.to eq(package2) }
+          end
+        end
+
+        context 'with maven_packages_group_level_improvements disabled' do
+          before do
+            stub_feature_flags(maven_packages_group_level_improvements: false)
+            expect(finder).to receive(:versionless_package?).and_call_original
+          end
+
+          context 'without order by package file' do
+            it { is_expected.to eq(package2) }
+          end
+
+          context 'with order by package file' do
+            let(:param_order_by_package_file) { true }
+
+            it { is_expected.to eq(package2) }
+          end
+        end
       end
     end
   end

spec/lib/gitlab/ci/pipeline/chain/validate/external_spec.rb

@@ -62,11 +62,42 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Validate::External do
     it 'respects the defined payload schema' do
       expect(::Gitlab::HTTP).to receive(:post) do |_url, params|
         expect(params[:body]).to match_schema('/external_validation')
+        expect(params[:timeout]).to eq(described_class::DEFAULT_VALIDATION_REQUEST_TIMEOUT)
       end
 
       perform!
     end
 
+    context 'with EXTERNAL_VALIDATION_SERVICE_TIMEOUT defined' do
+      before do
+        stub_env('EXTERNAL_VALIDATION_SERVICE_TIMEOUT', validation_service_timeout)
+      end
+
+      context 'with valid value' do
+        let(:validation_service_timeout) { '1' }
+
+        it 'uses defined timeout' do
+          expect(::Gitlab::HTTP).to receive(:post) do |_url, params|
+            expect(params[:timeout]).to eq(1)
+          end
+
+          perform!
+        end
+      end
+
+      context 'with invalid value' do
+        let(:validation_service_timeout) { '??' }
+
+        it 'uses default timeout' do
+          expect(::Gitlab::HTTP).to receive(:post) do |_url, params|
+            expect(params[:timeout]).to eq(described_class::DEFAULT_VALIDATION_REQUEST_TIMEOUT)
+          end
+
+          perform!
+        end
+      end
+    end
+
     shared_examples 'successful external authorization' do
       it 'does not drop the pipeline' do
         perform!

spec/models/packages/package_spec.rb

@@ -99,6 +99,34 @@ RSpec.describe Packages::Package, type: :model do
     end
   end
 
+  describe '.for_projects' do
+    let_it_be(:package1) { create(:maven_package) }
+    let_it_be(:package2) { create(:maven_package) }
+    let_it_be(:package3) { create(:maven_package) }
+
+    let(:projects) { ::Project.id_in([package1.project_id, package2.project_id]) }
+
+    subject { described_class.for_projects(projects.select(:id)) }
+
+    it 'returns package1 and package2' do
+      expect(projects).not_to receive(:any?)
+
+      expect(subject).to match_array([package1, package2])
+    end
+
+    context 'with maven_packages_group_level_improvements disabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: false)
+      end
+
+      it 'returns package1 and package2' do
+        expect(projects).to receive(:any?).and_call_original
+
+        expect(subject).to match_array([package1, package2])
+      end
+    end
+  end
+
   describe 'validations' do
     subject { build(:package) }
 

spec/models/user_callout_spec.rb

@@ -18,36 +18,14 @@ RSpec.describe UserCallout do
     it { is_expected.to validate_uniqueness_of(:feature_name).scoped_to(:user_id).ignoring_case_sensitivity }
   end
 
-  describe 'scopes' do
-    describe '.with_feature_name' do
-      let(:second_feature_name) { described_class.feature_names.keys.second }
-      let(:last_feature_name) { described_class.feature_names.keys.last }
-
-      it 'returns callout for requested feature name only' do
-        callout1 = create(:user_callout, feature_name: second_feature_name )
-        create(:user_callout, feature_name: last_feature_name )
-
-        callouts = described_class.with_feature_name(second_feature_name)
-
-        expect(callouts).to match_array([callout1])
-      end
-    end
-
-    describe '.with_dismissed_after' do
-      let(:some_feature_name) { described_class.feature_names.keys.second }
-      let(:callout_dismissed_month_ago) { create(:user_callout, feature_name: some_feature_name, dismissed_at: 1.month.ago )}
-
-      it 'does not return callouts dismissed before specified date' do
-        callouts = described_class.with_dismissed_after(15.days.ago)
-
-        expect(callouts).to match_array([])
-      end
-
-      it 'returns callouts dismissed after specified date' do
-        callouts = described_class.with_dismissed_after(2.months.ago)
-
-        expect(callouts).to match_array([callout_dismissed_month_ago])
-      end
+  describe '#dismissed_after?' do
+    let(:some_feature_name) { described_class.feature_names.keys.second }
+    let(:callout_dismissed_month_ago) { create(:user_callout, feature_name: some_feature_name, dismissed_at: 1.month.ago )}
+    let(:callout_dismissed_day_ago) { create(:user_callout, feature_name: some_feature_name, dismissed_at: 1.day.ago )}
+
+    it 'returns whether a callout dismissed after specified date' do
+      expect(callout_dismissed_month_ago.dismissed_after?(15.days.ago)).to eq(false)
+      expect(callout_dismissed_day_ago.dismissed_after?(15.days.ago)).to eq(true)
     end
   end
 end