Automatic merge of gitlab-org/gitlab master

app/controllers/projects/merge_requests_controller.rb

@@ -49,10 +49,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:diff_searching_usage_data, @project, default_enabled: :yaml)
   end
 
-  before_action do
-    push_frontend_feature_flag(:show_relevant_approval_rule_approvers, @project, default_enabled: :yaml)
-  end
-
   around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :discussions]
 
   after_action :log_merge_request_show, only: [:show]

app/services/projects/destroy_service.rb

@@ -41,6 +41,8 @@ module Projects
 
       true
     rescue StandardError => error
+      context = Gitlab::ApplicationContext.current.merge(project_id: project.id)
+      Gitlab::ErrorTracking.track_exception(error, **context)
       attempt_rollback(project, error.message)
       false
     rescue Exception => error # rubocop:disable Lint/RescueException

config/feature_flags/development/show_relevant_approval_rule_approvers.yml

----
-name: show_relevant_approval_rule_approvers
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60339
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/329153
-milestone: '13.12'
-type: development
-group: group::source code
-default_enabled: true

ee/app/assets/javascripts/approvals/components/approvers_select.vue

@@ -80,11 +80,6 @@ export default {
       default: false,
     },
   },
-  computed: {
-    isFeatureEnabled() {
-      return Boolean(gon.features?.showRelevantApprovalRuleApprovers);
-    },
-  },
   watch: {
     value(val) {
       if (val.length > 0) {
@@ -140,26 +135,15 @@ export default {
         .then((results) => ({ results }));
     },
     fetchGroups(term) {
-      if (this.isFeatureEnabled) {
-        const hasTerm = term.trim().length > 0;
-        const DEVELOPER_ACCESS_LEVEL = 30;
-
-        return Api.projectGroups(this.projectId, {
-          skip_groups: this.skipGroupIds,
-          ...(hasTerm ? { search: term } : {}),
-          with_shared: true,
-          shared_visible_only: true,
-          shared_min_access_level: DEVELOPER_ACCESS_LEVEL,
-        });
-      }
-
-      // Don't includeAll when search is empty. Otherwise, the user could get a lot of garbage choices.
-      // https://gitlab.com/gitlab-org/gitlab/issues/11566
-      const includeAll = term.trim().length > 0;
+      const hasTerm = term.trim().length > 0;
+      const DEVELOPER_ACCESS_LEVEL = 30;
 
-      return Api.groups(term, {
+      return Api.projectGroups(this.projectId, {
         skip_groups: this.skipGroupIds,
-        all_available: includeAll,
+        ...(hasTerm ? { search: term } : {}),
+        with_shared: true,
+        shared_visible_only: true,
+        shared_min_access_level: DEVELOPER_ACCESS_LEVEL,
       });
     },
     fetchUsers(term) {

ee/app/models/ci/minutes/context.rb

@@ -3,15 +3,13 @@
 module Ci
   module Minutes
     class Context
-      delegate :shared_runners_minutes_limit_enabled?, to: :level
+      delegate :shared_runners_minutes_limit_enabled?, to: :namespace
       delegate :name, to: :namespace, prefix: true
 
-      attr_reader :level
+      attr_reader :namespace
 
       def initialize(project, namespace, tracking_strategy: nil)
-        @project = project
         @namespace = project&.shared_runners_limit_namespace || namespace
-        @level = project || namespace
         @tracking_strategy = tracking_strategy
       end
 
@@ -21,8 +19,6 @@ module Ci
 
       private
 
-      attr_reader :project, :namespace
-
       def quota
         @quota ||= ::Ci::Minutes::Quota.new(namespace, tracking_strategy: @tracking_strategy)
       end

ee/app/models/ci/minutes/notification.rb

@@ -17,10 +17,10 @@ module Ci
 
       def show?(current_user, cookies = false)
         return false unless @stage
-        return false unless @context.level
+        return false unless @context.namespace
         return false if alert_has_been_dismissed?(cookies)
 
-        Ability.allowed?(current_user, :read_ci_minutes_quota, @context.level)
+        Ability.allowed?(current_user, :read_ci_minutes_quota, @context.namespace)
       end
 
       def text

ee/app/policies/ee/group_policy.rb

@@ -298,7 +298,6 @@ module EE
       rule { developer }.policy do
         enable :create_wiki
         enable :admin_merge_request
-        enable :read_ci_minutes_quota
         enable :read_group_audit_events
       end
 
@@ -316,6 +315,7 @@ module EE
         enable :read_group_compliance_dashboard
         enable :read_group_credentials_inventory
         enable :admin_group_credentials_inventory
+        enable :read_ci_minutes_quota
       end
 
       rule { (admin | owner) & group_merge_request_approval_settings_enabled }.policy do

ee/app/policies/ee/project_policy.rb

@@ -183,11 +183,14 @@ module EE
         enable :create_vulnerability_feedback
         enable :destroy_vulnerability_feedback
         enable :update_vulnerability_feedback
-        enable :read_ci_minutes_quota
         enable :admin_feature_flags_issue_links
         enable :read_project_audit_events
       end
 
+      rule { can?(:owner_access) }.policy do
+        enable :read_ci_minutes_quota
+      end
+
       rule { can?(:developer_access) & iterations_available }.policy do
         enable :create_iteration
         enable :admin_iteration

ee/spec/features/ci_shared_runner_warnings_spec.rb

@@ -13,153 +13,166 @@ RSpec.describe 'CI shared runner limits' do
   let!(:job) { create(:ci_build, pipeline: pipeline) }
 
   before do
+    group.add_user(user, membership_level)
     sign_in(user)
   end
 
-  context 'when on a project related page' do
-    before do
-      group.add_developer(user)
-    end
+  where(:membership_level, :visible) do
+    :owner | true
+    :developer | false
+  end
 
-    where(:case_name, :percent_threshold, :minutes_limit, :minutes_used) do
-      'warning level' | 30 | 1000 | 800
-      'danger level'  | 5  | 1000 | 980
-    end
+  with_them do
+    context 'when on a project related page' do
+      where(:membership_level, :visible) do
+        :owner | true
+        :developer | false
+      end
 
-    with_them do
-      context "when there is a notification and minutes still exist", :js do
-        let(:message) do
-          "#{group.name} namespace has #{percent_threshold}% or less Shared Runner Pipeline minutes remaining. " \
-            "After it runs out, no new jobs or pipelines in its projects will run."
-        end
+      before do
+        group.add_user(user, membership_level)
+      end
 
-        before do
-          group.update!(shared_runners_minutes_limit: minutes_limit)
-          allow_any_instance_of(::Ci::Minutes::Quota).to receive(:total_minutes_used).and_return(minutes_used)
-        end
+      where(:case_name, :percent_threshold, :minutes_limit, :minutes_used) do
+        'warning level' | 30 | 1000 | 800
+        'danger level'  | 5  | 1000 | 980
+      end
 
-        it 'displays a warning message on pipelines page' do
-          visit project_pipelines_path(project)
+      with_them do
+        context "when there is a notification and minutes still exist", :js do
+          let(:message) do
+            "#{group.name} namespace has #{percent_threshold}% or less Shared Runner Pipeline minutes remaining. " \
+              "After it runs out, no new jobs or pipelines in its projects will run."
+          end
+
+          before do
+            group.update!(shared_runners_minutes_limit: minutes_limit)
+            allow_any_instance_of(::Ci::Minutes::Quota).to receive(:total_minutes_used).and_return(minutes_used)
+          end
+
+          it 'displays a warning message on pipelines page' do
+            visit project_pipelines_path(project)
+
+            alerts_according_to_role(visible: visible, message: message)
+          end
+
+          it 'displays a warning message on project homepage' do
+            visit project_path(project)
 
-          expect_quota_exceeded_alert(message)
+            alerts_according_to_role(visible: visible, message: message)
+          end
+
+          it 'displays a warning message on a job page' do
+            visit project_job_path(project, job)
+
+            alerts_according_to_role(visible: visible, message: message)
+          end
+        end
+      end
+
+      context 'when limit is exceeded', :js do
+        let(:group) { create(:group, :with_used_build_minutes_limit) }
+        let(:message) do
+          "#{group.name} namespace has exceeded its pipeline minutes quota. " \
+            "Buy additional pipeline minutes, or no new jobs or pipelines in its projects will run."
         end
 
         it 'displays a warning message on project homepage' do
           visit project_path(project)
 
-          expect_quota_exceeded_alert(message)
+          alerts_according_to_role(visible: visible, message: message)
+        end
+
+        it 'displays a warning message on pipelines page' do
+          visit project_pipelines_path(project)
+
+          alerts_according_to_role(visible: visible, message: message)
         end
 
         it 'displays a warning message on a job page' do
           visit project_job_path(project, job)
 
-          expect_quota_exceeded_alert(message)
+          alerts_according_to_role(visible: visible, message: message)
         end
       end
-    end
 
-    context 'when limit is exceeded', :js do
-      let(:group) { create(:group, :with_used_build_minutes_limit) }
-      let(:message) do
-        "#{group.name} namespace has exceeded its pipeline minutes quota. " \
-          "Buy additional pipeline minutes, or no new jobs or pipelines in its projects will run."
-      end
+      context 'when limit not yet exceeded' do
+        let(:group) { create(:group, :with_not_used_build_minutes_limit) }
 
-      it 'displays a warning message on project homepage' do
-        visit project_path(project)
+        it 'does not display a warning message on project homepage' do
+          visit project_path(project)
 
-        expect_quota_exceeded_alert(message)
-      end
+          expect_no_quota_exceeded_alert
+        end
 
-      it 'displays a warning message on pipelines page' do
-        visit project_pipelines_path(project)
+        it 'does not display a warning message on pipelines page' do
+          visit project_pipelines_path(project)
 
-        expect_quota_exceeded_alert(message)
-      end
+          expect_no_quota_exceeded_alert
+        end
 
-      it 'displays a warning message on a job page' do
-        visit project_job_path(project, job)
+        it 'displays a warning message on a job page' do
+          visit project_job_path(project, job)
 
-        expect_quota_exceeded_alert(message)
+          expect_no_quota_exceeded_alert
+        end
       end
     end
 
-    context 'when limit not yet exceeded' do
-      let(:group) { create(:group, :with_not_used_build_minutes_limit) }
-
-      it 'does not display a warning message on project homepage' do
-        visit project_path(project)
-
-        expect_no_quota_exceeded_alert
+    context 'when on a group related page' do
+      where(:case_name, :percent_threshold, :minutes_limit, :minutes_used) do
+        'warning level' | 30 | 1000 | 800
+        'danger level'  | 5  | 1000 | 980
       end
 
-      it 'does not display a warning message on pipelines page' do
-        visit project_pipelines_path(project)
+      with_them do
+        context "when there is a notification and minutes still exist", :js do
+          let(:message) do
+            "#{group.name} namespace has #{percent_threshold}% or less Shared Runner Pipeline minutes remaining. " \
+              "After it runs out, no new jobs or pipelines in its projects will run."
+          end
 
-        expect_no_quota_exceeded_alert
-      end
+          before do
+            group.update!(shared_runners_minutes_limit: minutes_limit)
+            allow_any_instance_of(::Ci::Minutes::Quota).to receive(:total_minutes_used).and_return(minutes_used)
+          end
 
-      it 'displays a warning message on a job page' do
-        visit project_job_path(project, job)
+          it 'displays a warning message on group information page' do
+            visit group_path(group)
 
-        expect_no_quota_exceeded_alert
+            alerts_according_to_role(visible: visible, message: message)
+          end
+        end
       end
-    end
-  end
 
-  context 'when on a group related page' do
-    before do
-      group.add_owner(user)
-    end
-
-    where(:case_name, :percent_threshold, :minutes_limit, :minutes_used) do
-      'warning level' | 30 | 1000 | 800
-      'danger level'  | 5  | 1000 | 980
-    end
-
-    with_them do
-      context "when there is a notification and minutes still exist", :js do
+      context 'when limit is exceeded', :js do
+        let(:group) { create(:group, :with_used_build_minutes_limit) }
         let(:message) do
-          "#{group.name} namespace has #{percent_threshold}% or less Shared Runner Pipeline minutes remaining. " \
-            "After it runs out, no new jobs or pipelines in its projects will run."
-        end
-
-        before do
-          group.update!(shared_runners_minutes_limit: minutes_limit)
-          allow_any_instance_of(::Ci::Minutes::Quota).to receive(:total_minutes_used).and_return(minutes_used)
+          "#{group.name} namespace has exceeded its pipeline minutes quota. " \
+            "Buy additional pipeline minutes, or no new jobs or pipelines in its projects will run."
         end
 
         it 'displays a warning message on group information page' do
           visit group_path(group)
 
-          expect_quota_exceeded_alert(message)
+          alerts_according_to_role(visible: visible, message: message)
         end
       end
-    end
 
-    context 'when limit is exceeded', :js do
-      let(:group) { create(:group, :with_used_build_minutes_limit) }
-      let(:message) do
-        "#{group.name} namespace has exceeded its pipeline minutes quota. " \
-          "Buy additional pipeline minutes, or no new jobs or pipelines in its projects will run."
-      end
+      context 'when limit not yet exceeded' do
+        let(:group) { create(:group, :with_not_used_build_minutes_limit) }
 
-      it 'displays a warning message on group information page' do
-        visit group_path(group)
+        it 'does not display a warning message on group information page' do
+          visit group_path(group)
 
-        expect_quota_exceeded_alert(message)
+          expect_no_quota_exceeded_alert
+        end
       end
     end
+  end
 
-    context 'when limit not yet exceeded' do
-      let(:group) { create(:group, :with_not_used_build_minutes_limit) }
-
-      it 'does not display a warning message on group information page' do
-        visit group_path(group)
-
-        expect_no_quota_exceeded_alert
-      end
-    end
+  def alerts_according_to_role(visible: false, message: '')
+    visible ? expect_quota_exceeded_alert(message) : expect_no_quota_exceeded_alert
   end
 
   def expect_quota_exceeded_alert(message)

ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb

@@ -74,30 +74,6 @@ RSpec.describe 'Merge request > User edits MR with approval rules', :js do
     expect(page_rule_names.last).to have_text(rule_name)
   end
 
-  context 'with show_relevant_approval_rule_approvers feature flag disabled' do
-    before do
-      stub_feature_flags(show_relevant_approval_rule_approvers: false)
-    end
-
-    context "with public group" do
-      let(:group) { create(:group, :public) }
-
-      before do
-        group.add_developer create(:user)
-
-        click_button 'Approval rules'
-        click_button "Add approval rule"
-      end
-
-      it "with empty search, does not show public group" do
-        open_select2 members_selector
-        wait_for_requests
-
-        expect(page).not_to have_selector('.select2-result-label .group-result', text: group.name)
-      end
-    end
-  end
-
   context 'with public group' do
     let(:group) { create(:group, :public) }
 

ee/spec/features/merge_request/user_sets_approvers_spec.rb

@@ -55,20 +55,21 @@ RSpec.describe 'Merge request > User sets approvers', :js do
   end
 
   context "Group approvers" do
+    let(:project) { create(:project, :public, :repository, group: group) }
+    let(:group) { create(:group) }
+
     context 'when creating an MR' do
       let(:other_user) { create(:user) }
 
       before do
         project.add_developer(user)
         project.add_developer(other_user)
+        group.add_developer(other_user)
 
         sign_in(user)
       end
 
       it 'allows setting groups as approvers' do
-        group = create :group
-        group.add_developer(other_user)
-
         visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature' })
 
         open_modal(text: 'Add approval rule')
@@ -99,8 +100,6 @@ RSpec.describe 'Merge request > User sets approvers', :js do
       it 'allows delete approvers group when it is set in project' do
         approver = create :user
         project.add_developer(approver)
-        group = create :group
-        group.add_developer(other_user)
         group.add_developer(approver)
         create :approval_project_rule, project: project, users: [approver], groups: [group], approvals_required: 1
 
@@ -134,42 +133,6 @@ RSpec.describe 'Merge request > User sets approvers', :js do
         sign_in(user)
       end
 
-      context 'with show_relevant_approval_rule_approvers feature flag disabled' do
-        before do
-          stub_feature_flags(show_relevant_approval_rule_approvers: false)
-        end
-
-        it 'allows setting groups as approvers' do
-          group = create :group
-          group.add_developer(other_user)
-
-          visit edit_project_merge_request_path(project, merge_request)
-
-          open_modal(text: 'Add approval rule')
-          open_approver_select
-
-          expect(find('.select2-results')).not_to have_content(group.name)
-
-          close_approver_select
-          group.add_developer(user) # only display groups that user has access to
-          open_approver_select
-
-          expect(find('.select2-results')).to have_content(group.name)
-
-          find('.select2-results .user-result', text: group.name).click
-          close_approver_select
-          within('.modal-content') do
-            click_button 'Add approval rule'
-          end
-
-          click_on("Save changes")
-          wait_for_all_requests
-
-          expect(page).to have_content("Requires 1 approval from eligible users.")
-          expect(page).to have_selector("img[alt='#{other_user.name}']")
-        end
-      end
-
       it 'allows setting groups as approvers when there is possible group approvers' do
         group = create :group
         group_project = create(:project, :public, :repository, namespace: group)

ee/spec/features/projects/settings/merge_request_approvals_settings_spec.rb

@@ -6,8 +6,8 @@ RSpec.describe 'Project settings > [EE] Merge Request Approvals', :js do
   include FeatureApprovalHelper
 
   let_it_be(:user) { create(:user) }
-  let_it_be(:project) { create(:project) }
   let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, namespace: group) }
   let_it_be(:group_member) { create(:user) }
   let_it_be(:non_member) { create(:user) }
   let_it_be(:config_selector) { '.js-approval-rules' }

ee/spec/frontend/approvals/components/approvers_select_spec.js

@@ -73,7 +73,7 @@ describe('Approvals ApproversSelect', () => {
   };
 
   beforeEach(() => {
-    jest.spyOn(Api, 'groups').mockResolvedValue(TEST_GROUPS);
+    jest.spyOn(Api, 'projectGroups').mockResolvedValue(TEST_GROUPS);
     jest.spyOn(Api, 'projectUsers').mockReturnValue(Promise.resolve(TEST_USERS));
   });
 
@@ -123,7 +123,13 @@ describe('Approvals ApproversSelect', () => {
     });
 
     it('fetches all available groups', () => {
-      expect(Api.groups).toHaveBeenCalledWith(term, { skip_groups: [], all_available: true });
+      expect(Api.projectGroups).toHaveBeenCalledWith(TEST_PROJECT_ID, {
+        skip_groups: [],
+        search: term,
+        with_shared: true,
+        shared_visible_only: true,
+        shared_min_access_level: 30,
+      });
     });
 
     it('fetches users', () => {
@@ -154,9 +160,11 @@ describe('Approvals ApproversSelect', () => {
     });
 
     it('skips groups and does not fetch all available', () => {
-      expect(Api.groups).toHaveBeenCalledWith('', {
+      expect(Api.projectGroups).toHaveBeenCalledWith(TEST_PROJECT_ID, {
+        shared_min_access_level: 30,
+        shared_visible_only: true,
         skip_groups: skipGroupIds,
-        all_available: false,
+        with_shared: true,
       });
     });
 

ee/spec/models/ci/minutes/context_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Ci::Minutes::Context do
   describe 'delegation' do
     subject { described_class.new(project, group) }
 
-    it { is_expected.to delegate_method(:shared_runners_minutes_limit_enabled?).to(:level) }
+    it { is_expected.to delegate_method(:shared_runners_minutes_limit_enabled?).to(:namespace) }
     it { is_expected.to delegate_method(:name).to(:namespace).with_prefix }
   end
 end

ee/spec/models/ci/minutes/notification_spec.rb

@@ -189,7 +189,7 @@ RSpec.describe Ci::Minutes::Notification do
   context 'when at project level' do
     context 'when eligible to see notifications' do
       before do
-        group.add_developer(user)
+        group.add_owner(user)
       end
 
       describe '#show?' do
@@ -217,12 +217,22 @@ RSpec.describe Ci::Minutes::Notification do
         subject { described_class.new(injected_project, nil) }
       end
     end
+
+    context 'when user is not in the correct role' do
+      before do
+        group.add_developer user
+      end
+
+      it_behaves_like 'not eligible to see notifications' do
+        subject { described_class.new(injected_project, nil) }
+      end
+    end
   end
 
   context 'when at namespace level' do
     context 'when eligible to see notifications' do
       before do
-        group.add_developer(user)
+        group.add_owner(user)
       end
 
       context 'with a project that has runners enabled inside namespace' do
@@ -259,5 +269,15 @@ RSpec.describe Ci::Minutes::Notification do
         subject { described_class.new(injected_project, nil) }
       end
     end
+
+    context 'when user is not in the correct role' do
+      before do
+        group.add_developer user
+      end
+
+      it_behaves_like 'not eligible to see notifications' do
+        subject { described_class.new(injected_project, nil) }
+      end
+    end
   end
 end

ee/spec/policies/group_policy_spec.rb

@@ -1354,8 +1354,8 @@ RSpec.describe GroupPolicy do
     where(:role, :admin_mode, :allowed) do
       :guest      | nil   | false
       :reporter   | nil   | false
-      :developer  | nil   | true
-      :maintainer | nil   | true
+      :developer  | nil   | false
+      :maintainer | nil   | false
       :owner      | nil   | true
       :admin      | true  | true
       :admin      | false | false

ee/spec/policies/project_policy_spec.rb

@@ -1566,8 +1566,8 @@ RSpec.describe ProjectPolicy do
     where(:role, :admin_mode, :allowed) do
       :guest      | nil   | false
       :reporter   | nil   | false
-      :developer  | nil   | true
-      :maintainer | nil   | true
+      :developer  | nil   | false
+      :maintainer | nil   | false
       :owner      | nil   | true
       :admin      | false | false
       :admin      | true  | true

lib/gitlab/git/rugged_impl/commit.rb

@@ -31,6 +31,9 @@ module Gitlab
             oids.map { |oid| rugged_find(repo, oid) }
               .compact
               .map { |commit| decorate(repo, commit) }
+          # Match Gitaly's list_commits_by_oid behavior
+          rescue ::Gitlab::Git::Repository::NoRepository
+            []
           end
 
           override :find_commit

spec/lib/gitlab/git/commit_spec.rb

@@ -486,6 +486,16 @@ RSpec.describe Gitlab::Git::Commit, :seed_helper do
           expect(commits.first.sha).to eq(SeedRepo::Commit::ID)
           expect(commits.second.sha).to eq(SeedRepo::FirstCommit::ID)
         end
+
+        context 'when repo does not exist' do
+          let(:no_repository) { Gitlab::Git::Repository.new('default', '@does-not-exist/project', '', 'bogus/project') }
+
+          it 'returns empty commits' do
+            commits = described_class.batch_by_oid(no_repository, oids)
+
+            expect(commits.count).to eq(0)
+          end
+        end
       end
 
       context 'when oids is empty' do

spec/services/projects/destroy_service_spec.rb

@@ -78,6 +78,11 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do
       end.not_to raise_error
     end
 
+    it 'reports the error' do
+      expect(Gitlab::ErrorTracking).to receive(:track_exception).and_call_original
+      destroy_project(project, user, {})
+    end
+
     it 'unmarks the project as "pending deletion"' do
       destroy_project(project, user, {})