Automatic merge of gitlab-org/gitlab master

app/assets/javascripts/alert_handler.js

-// This allows us to dismiss alerts that we've migrated from bootstrap
-// Note: This ONLY works on alerts that are created on page load
+// This allows us to dismiss alerts and banners that we've migrated from bootstrap
+// Note: This ONLY works on elements that are created on page load
 // You can follow this effort in the following epic
 // https://gitlab.com/groups/gitlab-org/-/epics/4070
 
 export default function initAlertHandler() {
-  const ALERT_SELECTOR = '.gl-alert';
-  const CLOSE_SELECTOR = '.gl-alert-dismiss';
+  const DISMISSIBLE_SELECTORS = ['.gl-alert', '.gl-banner'];
+  const DISMISS_LABEL = '[aria-label="Dismiss"]';
+  const DISMISS_CLASS = '.gl-alert-dismiss';
 
-  const dismissAlert = ({ target }) => target.closest(ALERT_SELECTOR).remove();
-  const closeButtons = document.querySelectorAll(`${ALERT_SELECTOR} ${CLOSE_SELECTOR}`);
-  closeButtons.forEach(alert => alert.addEventListener('click', dismissAlert));
+  DISMISSIBLE_SELECTORS.forEach(selector => {
+    const elements = document.querySelectorAll(selector);
+    elements.forEach(element => {
+      const button = element.querySelector(DISMISS_LABEL) || element.querySelector(DISMISS_CLASS);
+      if (!button) {
+        return;
+      }
+      button.addEventListener('click', () => element.remove());
+    });
+  });
 }

app/graphql/resolvers/concerns/resolves_merge_requests.rb

@@ -40,7 +40,7 @@ module ResolvesMergeRequests
       author: [:author],
       merged_at: [:metrics],
       commit_count: [:metrics],
-      approved_by: [:approver_users],
+      approved_by: [:approved_by_users],
       milestone: [:milestone],
       head_pipeline: [:merge_request_diff, { head_pipeline: [:merge_request] }]
     }

app/graphql/types/merge_request_type.rb

@@ -174,10 +174,6 @@ module Types
     def commit_count
       object&.metrics&.commits_count
     end
-
-    def approvers
-      object.approver_users
-    end
   end
 end
 Types::MergeRequestType.prepend_if_ee('::EE::Types::MergeRequestType')

app/models/merge_request_context_commit.rb

@@ -22,8 +22,8 @@ class MergeRequestContextCommit < ApplicationRecord
   end
 
   # create MergeRequestContextCommit by given commit sha and it's diff file record
-  def self.bulk_insert(*args)
-    Gitlab::Database.bulk_insert('merge_request_context_commits', *args) # rubocop:disable Gitlab/BulkInsert
+  def self.bulk_insert(rows, **args)
+    Gitlab::Database.bulk_insert('merge_request_context_commits', rows, **args) # rubocop:disable Gitlab/BulkInsert
   end
 
   def to_commit

app/services/design_management/generate_image_versions_service.rb

@@ -48,6 +48,9 @@ module DesignManagement
       # Store and process the file
       action.image_v432x230.store!(raw_file)
       action.save!
+    rescue CarrierWave::IntegrityError => e
+      Gitlab::ErrorTracking.log_exception(e, project_id: project.id, design_id: action.design_id, version_id: action.version_id)
+      log_error(e.message)
     rescue CarrierWave::UploadError => e
       Gitlab::ErrorTracking.track_exception(e, project_id: project.id, design_id: action.design_id, version_id: action.version_id)
       log_error(e.message)

changelogs/unreleased/fix-wrong-scope-in-approved-by.yml

+---
+title: Fix approvedBy filed in MR GraphQL API
+merge_request: 43705
+author:
+type: fixed

changelogs/unreleased/gaga5lala-227175-carrierwave-error-handle.yml

+---
+title: Log CarrierWave::IntegrityError without sending exception
+merge_request: 43750
+author: gaga5lala
+type: other

config/locales/en.yml

@@ -20,6 +20,8 @@ en:
         token: "Grafana HTTP API Token"
         grafana_url: "Grafana API URL"
         grafana_enabled: "Grafana integration enabled"
+      service_desk_setting:
+        project_key: "Project name suffix"
       user/user_detail:
         job_title: 'Job title'
       user/user_detail:

doc/user/application_security/security_dashboard/index.md

@@ -174,14 +174,18 @@ thousands of vulnerabilities. Don't close the page until the download finishes.
 
 The fields in the export include:
 
+- Group Name
+- Project Name
 - Scanner Type
 - Scanner Name
 - Status
-- Name
+- Vulnerability
 - Details
+- Additional Info
 - Severity
 - [CVE](https://cve.mitre.org/)
-- Additional Info
+- [CWE](https://cwe.mitre.org/)
+- Other Identifiers
 
 ![Export vulnerabilities](img/instance_security_dashboard_export_csv_v13_4.png)
 

doc/user/compliance/license_compliance/index.md

@@ -127,6 +127,11 @@ is used to detect the languages/frameworks and in turn analyzes the licenses.
 The License Compliance settings can be changed through [environment variables](#available-variables) by using the
 [`variables`](../../../ci/yaml/README.md#variables) parameter in `.gitlab-ci.yml`.
 
+### When License Compliance runs
+
+When using the GitLab `License-Scanning.gitlab-ci.yml` template, the License Compliance job doesn't
+wait for other stages to complete.
+
 ### Available variables
 
 License Compliance can be configured using environment variables.

doc/user/permissions.md

@@ -386,6 +386,16 @@ with the permissions described on the documentation on [auditor users permission
 
 [Read more about Auditor users.](../administration/auditor_users.md)
 
+## Users with minimal access **(PREMIUM ONLY)**
+
+>[Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40942) in [GitLab Premium](https://about.gitlab.com/pricing/) 13.4.
+
+Administrators can add members with a "minimal access" role to a parent group. Such users don't
+automatically have access to projects and subgroups underneath. To support such access, administrators must explicitly add these "minimal access" users to the specific subgroups/projects.
+
+Users with minimal access can list the group in the UI and through the API. However, they cannot see
+details such as projects or subgroups. They do not have access to the group's page or list any of itssubgroups or projects.
+
 ## Project features
 
 Project features like wiki and issues can be hidden from users depending on

ee/app/graphql/ee/types/merge_request_type.rb

@@ -16,7 +16,7 @@ module EE
               description: 'Users who approved the merge request'
 
         def approved_by
-          object.approver_users
+          object.approved_by_users
         end
       end
     end

ee/lib/gitlab/ci/reports/security/finding.rb

@@ -74,17 +74,23 @@ module Gitlab
           def eql?(other)
             report_type == other.report_type &&
               location.fingerprint == other.location.fingerprint &&
-              primary_identifier.fingerprint == other.primary_identifier.fingerprint
+              primary_fingerprint == other.primary_fingerprint
           end
 
           def hash
-            report_type.hash ^ location.fingerprint.hash ^ primary_identifier.fingerprint.hash
+            report_type.hash ^ location.fingerprint.hash ^ primary_fingerprint.hash
           end
 
           def valid?
             scanner.present? && primary_identifier.present? && location.present?
           end
 
+          protected
+
+          def primary_fingerprint
+            primary_identifier&.fingerprint
+          end
+
           private
 
           def generate_project_fingerprint

ee/spec/factories/ci/reports/security/findings.rb

@@ -2,7 +2,7 @@
 
 FactoryBot.define do
   factory :ci_reports_security_finding, class: '::Gitlab::Ci::Reports::Security::Finding' do
-    compare_key { "#{identifiers.first.external_type}:#{identifiers.first.external_id}:#{location.fingerprint}" }
+    compare_key { "#{identifiers.first&.external_type}:#{identifiers.first&.external_id}:#{location.fingerprint}" }
     confidence { :medium }
     identifiers { Array.new(1) { FactoryBot.build(:ci_reports_security_identifier) } }
     location factory: :ci_reports_security_locations_sast

ee/spec/frontend/vue_shared/components/members/avatars/user_avatar_spec.js

+import { mount } from '@vue/test-utils';
+import { GlBadge } from '@gitlab/ui';
+import { member as memberMock } from 'jest/vue_shared/components/members/mock_data';
+import UserAvatar from '~/vue_shared/components/members/avatars/user_avatar.vue';
+
+describe('UserAvatar', () => {
+  let wrapper;
+
+  const createComponent = (propsData = {}) => {
+    wrapper = mount(UserAvatar, {
+      propsData: {
+        isCurrentUser: false,
+        ...propsData,
+      },
+    });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  describe('badges', () => {
+    it.each`
+      member                                          | badgeText
+      ${{ ...memberMock, usingLicense: true }}        | ${'Is using seat'}
+      ${{ ...memberMock, groupSso: true }}            | ${'SAML'}
+      ${{ ...memberMock, groupManagedAccount: true }} | ${'Managed Account'}
+    `('renders the "$badgeText" badge', ({ member, badgeText }) => {
+      createComponent({ member });
+
+      expect(wrapper.find(GlBadge).text()).toBe(badgeText);
+    });
+  });
+});

ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb

@@ -170,6 +170,14 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
 
     subject { finding.eql?(other_finding) }
 
+    context 'when the primary_identifier is nil' do
+      let(:identifier) { nil }
+
+      it 'does not raise an exception' do
+        expect { subject }.not_to raise_error
+      end
+    end
+
     context 'when the other finding has same `report_type`' do
       let(:report_type) { :sast }
 

ee/spec/requests/api/graphql/project/merge_requests_spec.rb

@@ -39,7 +39,7 @@ RSpec.describe 'getting merge request listings (EE) nested in a project' do
     let(:requested_fields) { query_graphql_field(:approved_by, nil, query_graphql_field(:nodes, nil, [:username])) }
 
     it 'exposes approver username' do
-      merge_request_a.approver_users << current_user
+      merge_request_a.approved_by_users << current_user
 
       execute_query
 

lib/gitlab/usage_data.rb

@@ -167,8 +167,7 @@ module Gitlab
             user_preferences_usage,
             ingress_modsecurity_usage,
             container_expiration_policies_usage,
-            service_desk_counts,
-            snowplow_event_counts
+            service_desk_counts
           ).tap do |data|
             data[:snippets] = data[:personal_snippets] + data[:project_snippets]
           end
@@ -176,7 +175,7 @@ module Gitlab
       end
       # rubocop: enable Metrics/AbcSize
 
-      def snowplow_event_counts(time_period: {})
+      def snowplow_event_counts(time_period)
         return {} unless report_snowplow_events?
 
         {

spec/frontend/alert_handler_spec.js

@@ -2,18 +2,26 @@ import { setHTMLFixture } from 'helpers/fixtures';
 import initAlertHandler from '~/alert_handler';
 
 describe('Alert Handler', () => {
-  const ALERT_SELECTOR = 'gl-alert';
-  const CLOSE_SELECTOR = 'gl-alert-dismiss';
-  const ALERT_HTML = `<div class="${ALERT_SELECTOR}"><button class="${CLOSE_SELECTOR}">Dismiss</button></div>`;
+  const ALERT_CLASS = 'gl-alert';
+  const BANNER_CLASS = 'gl-banner';
+  const DISMISS_CLASS = 'gl-alert-dismiss';
+  const DISMISS_LABEL = 'Dismiss';
 
-  const findFirstAlert = () => document.querySelector(`.${ALERT_SELECTOR}`);
-  const findAllAlerts = () => document.querySelectorAll(`.${ALERT_SELECTOR}`);
-  const findFirstCloseButton = () => document.querySelector(`.${CLOSE_SELECTOR}`);
+  const generateHtml = parentClass =>
+    `<div class="${parentClass}">
+      <button aria-label="${DISMISS_LABEL}">Dismiss</button>
+    </div>`;
+
+  const findFirstAlert = () => document.querySelector(`.${ALERT_CLASS}`);
+  const findFirstBanner = () => document.querySelector(`.${BANNER_CLASS}`);
+  const findAllAlerts = () => document.querySelectorAll(`.${ALERT_CLASS}`);
+  const findFirstDismissButton = () => document.querySelector(`[aria-label="${DISMISS_LABEL}"]`);
+  const findFirstDismissButtonByClass = () => document.querySelector(`.${DISMISS_CLASS}`);
 
   describe('initAlertHandler', () => {
     describe('with one alert', () => {
       beforeEach(() => {
-        setHTMLFixture(ALERT_HTML);
+        setHTMLFixture(generateHtml(ALERT_CLASS));
         initAlertHandler();
       });
 
@@ -22,14 +30,14 @@ describe('Alert Handler', () => {
       });
 
       it('should dismiss the alert on click', () => {
-        findFirstCloseButton().click();
+        findFirstDismissButton().click();
         expect(findFirstAlert()).not.toExist();
       });
     });
 
     describe('with two alerts', () => {
       beforeEach(() => {
-        setHTMLFixture(ALERT_HTML + ALERT_HTML);
+        setHTMLFixture(generateHtml(ALERT_CLASS) + generateHtml(ALERT_CLASS));
         initAlertHandler();
       });
 
@@ -38,9 +46,46 @@ describe('Alert Handler', () => {
       });
 
       it('should dismiss only one alert on click', () => {
-        findFirstCloseButton().click();
+        findFirstDismissButton().click();
         expect(findAllAlerts()).toHaveLength(1);
       });
     });
+
+    describe('with a dismissible banner', () => {
+      beforeEach(() => {
+        setHTMLFixture(generateHtml(BANNER_CLASS));
+        initAlertHandler();
+      });
+
+      it('should render the banner', () => {
+        expect(findFirstBanner()).toExist();
+      });
+
+      it('should dismiss the banner on click', () => {
+        findFirstDismissButton().click();
+        expect(findFirstBanner()).not.toExist();
+      });
+    });
+
+    // Dismiss buttons *should* have the correct aria labels, but some of them won't
+    // because legacy code isn't always a11y compliant.
+    // This tests that the fallback for the incorrectly labelled buttons works.
+    describe('with a mislabelled dismiss button', () => {
+      beforeEach(() => {
+        setHTMLFixture(`<div class="${ALERT_CLASS}">
+          <button class="${DISMISS_CLASS}">Dismiss</button>
+        </div>`);
+        initAlertHandler();
+      });
+
+      it('should render the banner', () => {
+        expect(findFirstAlert()).toExist();
+      });
+
+      it('should dismiss the banner on click', () => {
+        findFirstDismissButtonByClass().click();
+        expect(findFirstAlert()).not.toExist();
+      });
+    });
   });
 });

spec/frontend/vue_shared/components/members/avatars/user_avatar_spec.js

@@ -4,7 +4,7 @@ import { GlAvatarLink, GlBadge } from '@gitlab/ui';
 import { member as memberMock, orphanedMember } from '../mock_data';
 import UserAvatar from '~/vue_shared/components/members/avatars/user_avatar.vue';
 
-describe('MemberList', () => {
+describe('UserAvatar', () => {
   let wrapper;
 
   const { user } = memberMock;
@@ -68,11 +68,8 @@ describe('MemberList', () => {
   describe('badges', () => {
     it.each`
       member                                                                     | badgeText
-      ${{ ...memberMock, usingLicense: true }}                                   | ${'Is using seat'}
       ${{ ...memberMock, user: { ...memberMock.user, blocked: true } }}          | ${'Blocked'}
       ${{ ...memberMock, user: { ...memberMock.user, twoFactorEnabled: true } }} | ${'2FA'}
-      ${{ ...memberMock, groupSso: true }}                                       | ${'SAML'}
-      ${{ ...memberMock, groupManagedAccount: true }}                            | ${'Managed Account'}
     `('renders the "$badgeText" badge', ({ member, badgeText }) => {
       createComponent({ member });
 

spec/lib/gitlab/usage_data_spec.rb

@@ -1213,6 +1213,8 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
   end
 
   describe '.snowplow_event_counts' do
+    let_it_be(:time_period) { { collector_tstamp: 8.days.ago..1.day.ago } }
+
     context 'when self-monitoring project exists' do
       let_it_be(:project) { create(:project) }
 
@@ -1225,14 +1227,14 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
           stub_feature_flags(product_analytics: project)
 
           create(:product_analytics_event, project: project, se_category: 'epics', se_action: 'promote')
-          create(:product_analytics_event, project: project, se_category: 'epics', se_action: 'promote', collector_tstamp: 28.days.ago)
+          create(:product_analytics_event, project: project, se_category: 'epics', se_action: 'promote', collector_tstamp: 2.days.ago)
+          create(:product_analytics_event, project: project, se_category: 'epics', se_action: 'promote', collector_tstamp: 9.days.ago)
+
+          create(:product_analytics_event, project: project, se_category: 'foo', se_action: 'bar', collector_tstamp: 2.days.ago)
         end
 
         it 'returns promoted_issues for the time period' do
-          expect(described_class.snowplow_event_counts[:promoted_issues]).to eq(2)
-          expect(described_class.snowplow_event_counts(
-            time_period: described_class.last_28_days_time_period(column: :collector_tstamp)
-          )[:promoted_issues]).to eq(1)
+          expect(described_class.snowplow_event_counts(time_period)[:promoted_issues]).to eq(1)
         end
       end
 
@@ -1242,14 +1244,14 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
         end
 
         it 'returns an empty hash' do
-          expect(described_class.snowplow_event_counts).to eq({})
+          expect(described_class.snowplow_event_counts(time_period)).to eq({})
         end
       end
     end
 
     context 'when self-monitoring project does not exist' do
       it 'returns an empty hash' do
-        expect(described_class.snowplow_event_counts).to eq({})
+        expect(described_class.snowplow_event_counts(time_period)).to eq({})
       end
     end
   end

spec/services/design_management/generate_image_versions_service_spec.rb

@@ -52,25 +52,50 @@ RSpec.describe DesignManagement::GenerateImageVersionsService do
     end
 
     context 'when an error is encountered when generating the image versions' do
-      before do
-        expect_next_instance_of(DesignManagement::DesignV432x230Uploader) do |uploader|
-          expect(uploader).to receive(:cache!).and_raise(CarrierWave::DownloadError, 'foo')
+      context "CarrierWave::IntegrityError" do
+        before do
+          expect_next_instance_of(DesignManagement::DesignV432x230Uploader) do |uploader|
+            expect(uploader).to receive(:cache!).and_raise(CarrierWave::IntegrityError, 'foo')
+          end
+        end
+
+        it 'logs the exception' do
+          expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
+            instance_of(CarrierWave::IntegrityError),
+            project_id: project.id, version_id: version.id, design_id: version.designs.first.id
+          )
+
+          described_class.new(version).execute
         end
-      end
 
-      it 'logs the error' do
-        expect(Gitlab::AppLogger).to receive(:error).with('foo')
+        it 'logs the error' do
+          expect(Gitlab::AppLogger).to receive(:error).with('foo')
 
-        described_class.new(version).execute
+          described_class.new(version).execute
+        end
       end
 
-      it 'tracks the error' do
-        expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
-          instance_of(CarrierWave::DownloadError),
-          project_id: project.id, version_id: version.id, design_id: version.designs.first.id
-        )
+      context "CarrierWave::UploadError" do
+        before do
+          expect_next_instance_of(DesignManagement::DesignV432x230Uploader) do |uploader|
+            expect(uploader).to receive(:cache!).and_raise(CarrierWave::UploadError, 'foo')
+          end
+        end
 
-        described_class.new(version).execute
+        it 'logs the error' do
+          expect(Gitlab::AppLogger).to receive(:error).with('foo')
+
+          described_class.new(version).execute
+        end
+
+        it 'tracks the error' do
+          expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
+            instance_of(CarrierWave::UploadError),
+            project_id: project.id, version_id: version.id, design_id: version.designs.first.id
+          )
+
+          described_class.new(version).execute
+        end
       end
     end
   end