Merge branch '234031-add-better-error-messages-to-add-project-graphql' into 'master'

Enhance error messages for Add Project to Security Dashboard

See merge request gitlab-org/gitlab!40607

ee/app/graphql/mutations/instance_security_dashboard/add_project.rb

@@ -5,7 +5,7 @@ module Mutations
     class AddProject < BaseMutation
       graphql_name 'AddProjectToSecurityDashboard'
 
-      authorize :read_vulnerability
+      authorize :developer_access
 
       field :project, Types::ProjectType,
             null: true,
@@ -18,10 +18,11 @@ module Mutations
       def resolve(id:)
         project = authorized_find!(id: id)
         result = add_project(project)
+        error_message = prepare_error_message(result, project)
 
         {
-          project: result ? project : nil,
-          errors: result ? [] : ['The project already belongs to your dashboard or you don\'t have permission to perform this action']
+          project: error_message ? nil : project,
+          errors: [error_message].compact
         }
       end
 
@@ -33,9 +34,20 @@ module Mutations
 
       def add_project(project)
         Dashboard::Projects::CreateService
-          .new(current_user, current_user.security_dashboard_projects, feature: :security_dashboard)
+          .new(current_user, current_user.security_dashboard_projects, ability: :read_vulnerability)
           .execute([project.id])
-          .then { |result| result.added_project_ids.include?(project.id) }
+      end
+
+      def prepare_error_message(result, project)
+        return if result.added_project_ids.include?(project.id)
+
+        if result.duplicate_project_ids.include?(project.id)
+          _('The project has already been added to your dashboard.')
+        elsif result.not_licensed_project_ids.include?(project.id)
+          _('Only projects created under a Gold license are available in Security Dashboards.')
+        else
+          _('Project was not found or you do not have permission to add this project to Security Dashboards.')
+        end
       end
     end
   end

ee/app/services/dashboard/projects/create_service.rb

@@ -3,31 +3,58 @@
 module Dashboard
   module Projects
     class CreateService
-      Result = Struct.new(:added_project_ids, :invalid_project_ids, :duplicate_project_ids)
+      Result = Struct.new(:added_project_ids, :not_found_project_ids, :not_licensed_project_ids, :duplicate_project_ids) do
+        def invalid_project_ids
+          not_found_project_ids + not_licensed_project_ids
+        end
+      end
 
-      def initialize(user, projects_relation, feature:)
+      def initialize(user, projects_relation, feature: nil, ability: nil)
         @user = user
         @projects_relation = projects_relation
         @feature = feature
+        @ability = ability
       end
 
       def execute(project_ids)
-        projects_to_add = load_projects(project_ids)
+        found_projects = find_projects(project_ids)
+        licensed_projects = select_available_projects(found_projects)
+
+        not_found = find_invalid_ids(found_projects, project_ids)
+        not_licensed = find_invalid_ids(licensed_projects, project_ids) - not_found
 
-        invalid = find_invalid_ids(projects_to_add, project_ids)
-        added, duplicate = add_projects(projects_to_add)
+        added, duplicate = add_projects(licensed_projects)
 
-        Result.new(added.map(&:id), invalid, duplicate.map(&:id))
+        Result.new(added.map(&:id), not_found, not_licensed, duplicate.map(&:id))
       end
 
       private
 
       attr_reader :feature,
+                  :ability,
                   :projects_relation,
                   :user
 
-      def load_projects(project_ids)
-        Dashboard::Projects::ListService.new(user, feature: feature).execute(project_ids)
+      def find_projects(project_ids)
+        ProjectsFinder.new(
+          current_user: user,
+          project_ids_relation: project_ids,
+          params: projects_finder_params
+        ).execute
+      end
+
+      def projects_finder_params
+        return {} if user.can?(:read_all_resources)
+
+        {
+          min_access_level: ProjectMember::DEVELOPER
+        }
+      end
+
+      def select_available_projects(projects)
+        projects
+          .select { |project| feature.blank? || project.feature_available?(feature) }
+          .select { |project| ability.blank? || user.can?(ability, project) }
       end
 
       def find_invalid_ids(projects_to_add, project_ids)

ee/changelogs/unreleased/234031-add-better-error-messages-to-add-project-graphql.yml

+---
+title: Enhance error messages for Add Project to Security Dashboard mutation
+merge_request: 40607
+author:
+type: changed

ee/spec/graphql/mutations/instance_security_dashboard/add_project_spec.rb

@@ -37,6 +37,18 @@ RSpec.describe Mutations::InstanceSecurityDashboard::AddProject do
         end
       end
 
+      context 'when security_dashboard is disabled for my project' do
+        context 'when project is not licensed to be added to the security dashboard' do
+          let(:selected_project) { my_project }
+
+          it 'does not add project to the security dashboard', :aggregate_failures do
+            expect(subject[:project]).to be_nil
+            expect(subject[:errors]).to include('Only projects created under a Gold license are available in Security Dashboards.')
+            expect(user.security_dashboard_projects).to include(already_added_project)
+          end
+        end
+      end
+
       context 'when security_dashboard is enabled' do
         before do
           stub_licensed_features(security_dashboard: true)
@@ -65,7 +77,7 @@ RSpec.describe Mutations::InstanceSecurityDashboard::AddProject do
 
           it 'does not add project to the security dashboard', :aggregate_failures do
             expect(subject[:project]).to be_nil
-            expect(subject[:errors]).to eq(['The project already belongs to your dashboard or you don\'t have permission to perform this action'])
+            expect(subject[:errors]).to include('The project has already been added to your dashboard.')
             expect(user.security_dashboard_projects).to include(already_added_project)
           end
         end

ee/spec/services/dashboard/projects/create_service_spec.rb

@@ -4,18 +4,24 @@ require 'spec_helper'
 
 RSpec.describe Dashboard::Projects::CreateService do
   let(:user) { create(:user) }
-  let(:service) { described_class.new(user, user.ops_dashboard_projects, feature: :operations_dashboard) }
+  let(:service) { described_class.new(user, user.ops_dashboard_projects, feature: feature, ability: ability) }
+  let(:feature) { :operations_dashboard }
+  let(:ability) { nil }
   let(:project) { create(:project) }
 
   describe '#execute' do
-    let(:projects_service) { double(Dashboard::Projects::ListService) }
+    let(:projects_finder) { double(ProjectsFinder) }
     let(:result) { service.execute(input) }
+    let(:feature_available) { true }
+    let(:permission_available) { false }
 
     before do
-      allow(Dashboard::Projects::ListService)
-        .to receive(:new).with(user, feature: :operations_dashboard).and_return(projects_service)
-      allow(projects_service)
-        .to receive(:execute).with(input).and_return(output)
+      allow(ProjectsFinder)
+        .to receive(:new).with(current_user: user, project_ids_relation: input, params: { min_access_level: ProjectMember::DEVELOPER }).and_return(projects_finder)
+      allow(projects_finder)
+        .to receive(:execute).and_return(output)
+      allow(project).to receive(:feature_available?).and_return(feature_available)
+      allow(user).to receive(:can?).and_return(permission_available)
     end
 
     context 'with projects' do
@@ -37,6 +43,60 @@ RSpec.describe Dashboard::Projects::CreateService do
         end
       end
 
+      context 'with a project that does not exist' do
+        let(:input) { [non_existing_record_id] }
+        let(:output) { [] }
+
+        it 'does not add a not found project' do
+          expect(result).to eq(expected_result(not_found_project_ids: [non_existing_record_id]))
+        end
+      end
+
+      context 'when feature name is provided' do
+        context 'with project without provided feature enabled' do
+          let(:input) { [project.id] }
+          let(:feature_available) { false }
+          let(:ability) { nil }
+
+          it 'checks if feature is available' do
+            expect(project).to receive(:feature_available?).and_return(false)
+            result
+          end
+
+          it 'does not check if user has access to the project with given ability' do
+            expect(user).not_to receive(:can?).with(ability, project)
+            result
+          end
+
+          it 'does not add a not licensed project' do
+            expect(result).to eq(expected_result(not_licensed_project_ids: [project.id]))
+          end
+        end
+      end
+
+      context 'when ability name is provided' do
+        context 'with project for which user has no permission' do
+          let(:input) { [project.id] }
+          let(:feature) { nil }
+          let(:ability) { :read_vulnerability }
+          let(:permission_available) { false }
+
+          it 'does not check if feature is available' do
+            expect(project).not_to receive(:feature_available?)
+            result
+          end
+
+          it 'checks if user has access to the project with given ability' do
+            expect(user).to receive(:can?).with(ability, project).and_return(false)
+            result
+          end
+
+          it 'does not add a not licensed project' do
+            expect(result).to eq(expected_result(not_licensed_project_ids: [project.id]))
+          end
+        end
+      end
+
       context 'with repeating project id' do
         let(:input) { [project.id, project.id] }
 
@@ -63,11 +123,12 @@ RSpec.describe Dashboard::Projects::CreateService do
 
   def expected_result(
     added_project_ids: [],
-    invalid_project_ids: [],
+    not_found_project_ids: [],
+    not_licensed_project_ids: [],
     duplicate_project_ids: []
   )
     described_class::Result.new(
-      added_project_ids, invalid_project_ids, duplicate_project_ids
+      added_project_ids, not_found_project_ids, not_licensed_project_ids, duplicate_project_ids
     )
   end
 end

locale/gitlab.pot

@@ -17212,6 +17212,9 @@ msgstr ""
 msgid "Only project members will be imported. Group members will be skipped."
 msgstr ""
 
+msgid "Only projects created under a Gold license are available in Security Dashboards."
+msgstr ""
+
 msgid "Only verified users with an email address in any of these domains can be added to the group."
 msgstr ""
 
@@ -19135,6 +19138,9 @@ msgstr ""
 msgid "Project visibility level will be changed to match namespace rules when transferring to a group."
 msgstr ""
 
+msgid "Project was not found or you do not have permission to add this project to Security Dashboards."
+msgstr ""
+
 msgid "Project: %{name}"
 msgstr ""
 
@@ -24689,6 +24695,9 @@ msgstr ""
 msgid "The project can be accessed without any authentication."
 msgstr ""
 
+msgid "The project has already been added to your dashboard."
+msgstr ""
+
 msgid "The project is accessible only by members of the project. Access must be granted explicitly to each user."
 msgstr ""