Merge branch '326229-package-displayable' into 'master'

Add displayable and installable scopes to package finders

See merge request gitlab-org/gitlab!59921

app/finders/concerns/packages/finder_helper.rb

@@ -9,12 +9,16 @@ module Packages
 
     private
 
+    def packages_for_project(project)
+      project.packages.installable
+    end
+
     def packages_visible_to_user(user, within_group:)
       return ::Packages::Package.none unless within_group
       return ::Packages::Package.none unless Ability.allowed?(user, :read_group, within_group)
 
       projects = projects_visible_to_reporters(user, within_group: within_group)
-      ::Packages::Package.for_projects(projects.select(:id))
+      ::Packages::Package.for_projects(projects.select(:id)).installable
     end
 
     def projects_visible_to_user(user, within_group:)

app/finders/packages/composer/packages_finder.rb

@@ -9,7 +9,7 @@ module Packages
       end
 
       def execute
-        packages_for_group_projects.composer.preload_composer
+        packages_for_group_projects(installable_only: true).composer.preload_composer
       end
     end
   end

app/finders/packages/conan/package_finder.rb

@@ -11,7 +11,7 @@ module Packages
       end
 
       def execute
-        packages_for_current_user.with_name_like(query).order_name_asc if query
+        packages_for_current_user.installable.with_name_like(query).order_name_asc if query
       end
 
       private

app/finders/packages/generic/package_finder.rb

@@ -11,6 +11,7 @@ module Packages
         project
           .packages
           .generic
+          .installable
           .by_name_and_version!(package_name, package_version)
       end
 

app/finders/packages/go/package_finder.rb

@@ -21,6 +21,7 @@ module Packages
         @project
           .packages
           .golang
+          .installable
           .with_name(@module_name)
           .with_version(@module_version)
       end

app/finders/packages/group_packages_finder.rb

@@ -20,7 +20,7 @@ module Packages
 
     attr_reader :current_user, :group, :params
 
-    def packages_for_group_projects
+    def packages_for_group_projects(installable_only: false)
       packages = ::Packages::Package
         .including_build_info
         .including_project_route
@@ -32,7 +32,7 @@ module Packages
       packages = filter_with_version(packages)
       packages = filter_by_package_type(packages)
       packages = filter_by_package_name(packages)
-      filter_by_status(packages)
+      installable_only ? packages.installable : filter_by_status(packages)
     end
 
     def group_projects_visible_to_current_user

app/finders/packages/maven/package_finder.rb

@@ -26,9 +26,9 @@ module Packages
 
       def base
         if @project
-          packages_for_a_single_project
+          packages_for_project(@project)
         elsif @group
-          packages_for_multiple_projects
+          packages_visible_to_user(@current_user, within_group: @group)
         else
           ::Packages::Package.none
         end
@@ -40,23 +40,6 @@ module Packages
 
         matching_packages
       end
-
-      # Produces a query that retrieves packages from a single project.
-      def packages_for_a_single_project
-        @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_visible_to_user(@current_user, within_group: @group)
-      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)
-      end
     end
   end
 end

app/finders/packages/npm/package_finder.rb

@@ -14,6 +14,7 @@ module Packages
       def execute
         base.npm
             .with_name(@package_name)
+            .installable
             .last_of_each_version
             .preload_files
       end

app/finders/packages/nuget/package_finder.rb

@@ -23,7 +23,7 @@ module Packages
 
       def base
         if project?
-          @project_or_group.packages
+          packages_for_project(@project_or_group)
         elsif group?
           packages_visible_to_user(@current_user, within_group: @project_or_group)
         else

app/finders/packages/package_finder.rb

@@ -12,6 +12,7 @@ module Packages
         .including_build_info
         .including_project_route
         .including_tags
+        .displayable
         .processed
         .find(@package_id)
     end

app/models/packages/package.rb

@@ -6,6 +6,7 @@ class Packages::Package < ApplicationRecord
   include Gitlab::Utils::StrongMemoize
 
   DISPLAYABLE_STATUSES = [:default, :error].freeze
+  INSTALLABLE_STATUSES = [:default].freeze
 
   belongs_to :project
   belongs_to :creator, class_name: 'User'
@@ -86,6 +87,7 @@ class Packages::Package < ApplicationRecord
   scope :with_package_type, ->(package_type) { where(package_type: package_type) }
   scope :with_status, ->(status) { where(status: status) }
   scope :displayable, -> { with_status(DISPLAYABLE_STATUSES) }
+  scope :installable, -> { with_status(INSTALLABLE_STATUSES) }
   scope :including_build_info, -> { includes(pipelines: :user) }
   scope :including_project_route, -> { includes(project: { namespace: :route }) }
   scope :including_tags, -> { includes(:tags) }

app/services/packages/nuget/search_service.rb

@@ -103,6 +103,7 @@ module Packages
 
       def nuget_packages
         Packages::Package.nuget
+                         .displayable
                          .has_version
                          .without_nuget_temporary_name
       end

changelogs/unreleased/326229-package-displayable.yml

+---
+title: Include installable and/or displayable packages only in package finders
+merge_request: 59921
+author:
+type: changed

spec/finders/concerns/packages/finder_helper_spec.rb

@@ -3,6 +3,30 @@
 require 'spec_helper'
 
 RSpec.describe ::Packages::FinderHelper do
+  describe '#packages_for_project' do
+    let_it_be_with_reload(:project1) { create(:project) }
+    let_it_be(:package1) { create(:package, project: project1) }
+    let_it_be(:package2) { create(:package, :error, project: project1) }
+    let_it_be(:project2) { create(:project) }
+    let_it_be(:package3) { create(:package, project: project2) }
+
+    let(:finder_class) do
+      Class.new do
+        include ::Packages::FinderHelper
+
+        def execute(project1)
+          packages_for_project(project1)
+        end
+      end
+    end
+
+    let(:finder) { finder_class.new }
+
+    subject { finder.execute(project1) }
+
+    it { is_expected.to eq [package1]}
+  end
+
   describe '#packages_visible_to_user' do
     using RSpec::Parameterized::TableSyntax
 
@@ -12,6 +36,7 @@ RSpec.describe ::Packages::FinderHelper do
     let_it_be_with_reload(:subgroup) { create(:group, parent: group) }
     let_it_be_with_reload(:project2) { create(:project, namespace: subgroup) }
     let_it_be(:package2) { create(:package, project: project2) }
+    let_it_be(:package3) { create(:package, :error, project: project2) }
 
     let(:finder_class) do
       Class.new do

spec/finders/packages/composer/packages_finder_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe ::Packages::Composer::PackagesFinder do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, group: group) }
+
+  let(:params) { {} }
+
+  describe '#execute' do
+    let_it_be(:composer_package) { create(:composer_package, project: project) }
+    let_it_be(:composer_package2) { create(:composer_package, project: project) }
+    let_it_be(:error_package) { create(:composer_package, :error, project: project) }
+    let_it_be(:composer_package3) { create(:composer_package) }
+
+    subject { described_class.new(user, group, params).execute }
+
+    before do
+      project.add_developer(user)
+    end
+
+    it { is_expected.to match_array([composer_package, composer_package2]) }
+  end
+end

spec/finders/packages/conan/package_finder_spec.rb

@@ -11,7 +11,8 @@ RSpec.describe ::Packages::Conan::PackageFinder do
 
     subject { described_class.new(user, query: query).execute }
 
-    context 'packages that are not visible to user' do
+    context 'packages that are not installable' do
+      let!(:conan_package3) { create(:conan_package, :error, project: project) }
       let!(:non_visible_project) { create(:project, :private) }
       let!(:non_visible_conan_package) { create(:conan_package, project: non_visible_project) }
       let(:query) { "#{conan_package.name.split('/').first[0, 3]}%" }

spec/finders/packages/generic/package_finder_spec.rb

@@ -23,6 +23,13 @@ RSpec.describe ::Packages::Generic::PackageFinder do
       expect(found_package).to eq(package)
     end
 
+    it 'does not find uninstallable packages' do
+      error_package = create(:generic_package, :error, project: project)
+
+      expect { finder.execute!(error_package.name, error_package.version) }
+        .to raise_error(ActiveRecord::RecordNotFound)
+    end
+
     it 'raises ActiveRecord::RecordNotFound if package is not found' do
       expect { finder.execute!(package.name, '3.1.4') }
         .to raise_error(ActiveRecord::RecordNotFound)

spec/finders/packages/go/package_finder_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Packages::Go::PackageFinder do
 
   let_it_be(:mod) { create :go_module, project: project }
   let_it_be(:version) { create :go_module_version, :tagged, mod: mod, name: 'v1.0.1' }
-  let_it_be(:package) { create :golang_package, project: project, name: mod.name, version: 'v1.0.1' }
+  let_it_be_with_refind(:package) { create :golang_package, project: project, name: mod.name, version: 'v1.0.1' }
 
   let(:finder) { described_class.new(project, mod_name, version_name) }
 
@@ -54,6 +54,17 @@ RSpec.describe Packages::Go::PackageFinder do
       it { is_expected.to eq(package) }
     end
 
+    context 'with an uninstallable package' do
+      let(:mod_name) { mod.name }
+      let(:version_name) { version.name }
+
+      before do
+        package.update_column(:status, 1)
+      end
+
+      it { is_expected.to eq(nil) }
+    end
+
     context 'with an invalid name' do
       let(:mod_name) { 'foo/bar' }
       let(:version_name) { 'baz' }

spec/finders/packages/maven/package_finder_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe ::Packages::Maven::PackageFinder do
   let_it_be(:user)    { create(:user) }
   let_it_be(:group)   { create(:group) }
   let_it_be(:project) { create(:project, namespace: group) }
-  let_it_be(:package) { create(:maven_package, project: project) }
+  let_it_be_with_refind(:package) { create(:maven_package, project: project) }
 
   let(:param_path) { nil }
   let(:param_project) { nil }
@@ -36,6 +36,16 @@ RSpec.describe ::Packages::Maven::PackageFinder do
             expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
           end
         end
+
+        context 'with an uninstallable package' do
+          let(:param_path) { package.maven_metadatum.path }
+
+          before do
+            package.update_column(:status, 1)
+          end
+
+          it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+        end
       end
 
       context 'within the project' do

spec/finders/packages/npm/package_finder_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 
 RSpec.describe ::Packages::Npm::PackageFinder do
   let_it_be_with_reload(:project) { create(:project)}
-  let_it_be(:package) { create(:npm_package, project: project) }
+  let_it_be_with_refind(:package) { create(:npm_package, project: project) }
 
   let(:project) { package.project }
   let(:package_name) { package.name }
@@ -46,6 +46,14 @@ RSpec.describe ::Packages::Npm::PackageFinder do
 
         it { is_expected.to be_empty }
       end
+
+      context 'with an uninstallable package' do
+        before do
+          package.update_column(:status, 1)
+        end
+
+        it { is_expected.to be_empty }
+      end
     end
 
     subject { finder.execute }

spec/finders/packages/nuget/package_finder_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Packages::Nuget::PackageFinder do
   let_it_be(:group) { create(:group) }
   let_it_be(:subgroup) { create(:group, parent: group) }
   let_it_be(:project) { create(:project, namespace: subgroup) }
-  let_it_be(:package1) { create(:nuget_package, project: project) }
+  let_it_be_with_refind(:package1) { create(:nuget_package, project: project) }
   let_it_be(:package2) { create(:nuget_package, name: package1.name, version: '2.0.0', project: project) }
   let_it_be(:package3) { create(:nuget_package, name: 'Another.Dummy.Package', project: project) }
   let_it_be(:other_package_1) { create(:nuget_package, name: package1.name, version: package1.version) }
@@ -33,6 +33,14 @@ RSpec.describe Packages::Nuget::PackageFinder do
         it { is_expected.to be_empty }
       end
 
+      context 'with an uninstallable package' do
+        before do
+          package1.update_column(:status, 1)
+        end
+
+        it { is_expected.to contain_exactly(package2) }
+      end
+
       context 'with valid version' do
         let(:package_version) { '2.0.0' }
 

spec/finders/packages/package_finder_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe ::Packages::PackageFinder do
   let_it_be(:project) { create(:project) }
-  let_it_be(:maven_package) { create(:maven_package, project: project) }
+  let_it_be_with_refind(:maven_package) { create(:maven_package, project: project) }
 
   describe '#execute' do
     let(:package_id) { maven_package.id }
@@ -13,6 +13,16 @@ RSpec.describe ::Packages::PackageFinder do
 
     it { is_expected.to eq(maven_package) }
 
+    context 'with non-displayable package' do
+      before do
+        maven_package.update_column(:status, 1)
+      end
+
+      it 'raises an exception' do
+        expect { subject }.to raise_exception(ActiveRecord::RecordNotFound)
+      end
+    end
+
     context 'processing packages' do
       let_it_be(:nuget_package) { create(:nuget_package, project: project, name: Packages::Nuget::TEMPORARY_PACKAGE_NAME) }
       let(:package_id) { nuget_package.id }

spec/models/packages/package_spec.rb

@@ -660,27 +660,37 @@ RSpec.describe Packages::Package, type: :model do
       it { is_expected.to match_array([pypi_package]) }
     end
 
-    describe '.displayable' do
+    context 'status scopes' do
       let_it_be(:hidden_package) { create(:maven_package, :hidden) }
       let_it_be(:processing_package) { create(:maven_package, :processing) }
       let_it_be(:error_package) { create(:maven_package, :error) }
 
-      subject { described_class.displayable }
+      describe '.displayable' do
+        subject { described_class.displayable }
 
-      it 'does not include non-displayable packages', :aggregate_failures do
-        is_expected.to include(error_package)
-        is_expected.not_to include(hidden_package)
-        is_expected.not_to include(processing_package)
+        it 'does not include non-displayable packages', :aggregate_failures do
+          is_expected.to include(error_package)
+          is_expected.not_to include(hidden_package)
+          is_expected.not_to include(processing_package)
+        end
       end
-    end
 
-    describe '.with_status' do
-      let_it_be(:hidden_package) { create(:maven_package, :hidden) }
+      describe '.installable' do
+        subject { described_class.installable }
 
-      subject { described_class.with_status(:hidden) }
+        it 'does not include non-displayable packages', :aggregate_failures do
+          is_expected.not_to include(error_package)
+          is_expected.not_to include(hidden_package)
+          is_expected.not_to include(processing_package)
+        end
+      end
+
+      describe '.with_status' do
+        subject { described_class.with_status(:hidden) }
 
-      it 'returns packages with specified status' do
-        is_expected.to match_array([hidden_package])
+        it 'returns packages with specified status' do
+          is_expected.to match_array([hidden_package])
+        end
       end
     end
   end

spec/services/packages/nuget/search_service_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Packages::Nuget::SearchService do
   let_it_be(:group) { create(:group) }
   let_it_be(:subgroup) { create(:group, parent: group) }
   let_it_be(:project) { create(:project, namespace: subgroup) }
-  let_it_be(:package_a) { create(:nuget_package, project: project, name: 'DummyPackageA') }
+  let_it_be_with_refind(:package_a) { create(:nuget_package, project: project, name: 'DummyPackageA') }
   let_it_be(:packages_b) { create_list(:nuget_package, 5, project: project, name: 'DummyPackageB') }
   let_it_be(:packages_c) { create_list(:nuget_package, 5, project: project, name: 'DummyPackageC') }
   let_it_be(:package_d) { create(:nuget_package, project: project, name: 'FooBarD') }
@@ -79,6 +79,16 @@ RSpec.describe Packages::Nuget::SearchService do
         it { expect_search_results 4, package_a, packages_b, packages_c, package_d }
       end
 
+      context 'with non-displayable packages' do
+        let(:search_term) { '' }
+
+        before do
+          package_a.update_column(:status, 1)
+        end
+
+        it { expect_search_results 3, packages_b, packages_c, package_d }
+      end
+
       context 'with prefix search term' do
         let(:search_term) { 'dummy' }
 

spec/support/shared_examples/finders/packages_shared_examples.rb

@@ -20,9 +20,11 @@ end
 
 RSpec.shared_examples 'concerning package statuses' do
   let_it_be(:hidden_package) { create(:maven_package, :hidden, project: project) }
+  let_it_be(:error_package) { create(:maven_package, :error, project: project) }
 
-  context 'hidden packages' do
+  context 'displayable packages' do
     it { is_expected.not_to include(hidden_package) }
+    it { is_expected.to include(error_package) }
   end
 
   context 'with status param' do