Automatic merge of gitlab-org/gitlab master

.gitlab/ci/frontend.gitlab-ci.yml

@@ -132,6 +132,7 @@ rspec frontend_fixture:
   extends:
     - .frontend-fixtures-base
     - .frontend:rules:default-frontend-jobs
+  parallel: 2
 
 rspec frontend_fixture as-if-foss:
   extends:

app/assets/javascripts/popovers/components/popovers.vue

 <script>
-// We can't use v-safe-html here as the popover's title or content might contains SVGs that would
-// be stripped by the directive's sanitizer. Instead, we fallback on v-html and we use GitLab's
-// dompurify config that lets SVGs be rendered properly.
-// Context: https://gitlab.com/gitlab-org/gitlab/-/issues/247207
-/* eslint-disable vue/no-v-html */
-import { GlPopover } from '@gitlab/ui';
-import { sanitize } from '~/lib/dompurify';
+import { GlPopover, GlSafeHtmlDirective } from '@gitlab/ui';
 
 const newPopover = (element) => {
   const { content, html, placement, title, triggers = 'focus' } = element.dataset;
@@ -24,6 +18,9 @@ export default {
   components: {
     GlPopover,
   },
+  directives: {
+    SafeHtml: GlSafeHtmlDirective,
+  },
   data() {
     return {
       popovers: [],
@@ -71,9 +68,9 @@ export default {
     popoverExists(element) {
       return this.popovers.some((popover) => popover.target === element);
     },
-    getSafeHtml(html) {
-      return sanitize(html);
-    },
+  },
+  safeHtmlConfig: {
+    ADD_TAGS: ['use'], // to support icon SVGs
   },
 };
 </script>
@@ -82,10 +79,10 @@ export default {
   <div>
     <gl-popover v-for="(popover, index) in popovers" :key="index" v-bind="popover">
       <template #title>
-        <span v-if="popover.html" v-html="getSafeHtml(popover.title)"></span>
+        <span v-if="popover.html" v-safe-html:[$options.safeHtmlConfig]="popover.title"></span>
         <span v-else>{{ popover.title }}</span>
       </template>
-      <span v-if="popover.html" v-html="getSafeHtml(popover.content)"></span>
+      <span v-if="popover.html" v-safe-html:[$options.safeHtmlConfig]="popover.content"></span>
       <span v-else>{{ popover.content }}</span>
     </gl-popover>
   </div>

app/models/error_tracking/error.rb

@@ -22,11 +22,15 @@ class ErrorTracking::Error < ApplicationRecord
   def self.report_error(name:, description:, actor:, platform:, timestamp:)
     safe_find_or_create_by(
       name: name,
-      description: description,
       actor: actor,
       platform: platform
-    ) do |error|
-      error.update!(last_seen_at: timestamp)
+    ).tap do |error|
+      error.update!(
+        # Description can contain object id, so it can't be
+        # used as a group criteria for similar errors.
+        description: description,
+        last_seen_at: timestamp
+      )
     end
   end
 

app/services/error_tracking/collect_error_service.rb

@@ -18,7 +18,7 @@ module ErrorTracking
       # Together with occured_at these are 2 main attributes that we need to save here.
       error.events.create!(
         environment: event['environment'],
-        description: exception['type'],
+        description: exception['value'],
         level: event['level'],
         occurred_at: event['timestamp'],
         payload: event

app/services/merge_requests/mergeability_check_service.rb

@@ -157,7 +157,9 @@ module MergeRequests
 
     def merge_to_ref
       params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) }
-      result = MergeRequests::MergeToRefService.new(project: project, current_user: merge_request.author, params: params).execute(merge_request)
+      result = MergeRequests::MergeToRefService
+        .new(project: project, current_user: merge_request.author, params: params)
+        .execute(merge_request, true)
 
       result[:status] == :success
     end

db/migrate/20210825104558_change_description_limit_error_tracking_event.rb

+# frozen_string_literal: true
+
+class ChangeDescriptionLimitErrorTrackingEvent < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    remove_text_limit :error_tracking_error_events, :description
+    add_text_limit :error_tracking_error_events, :description, 1024
+  end
+
+  def down
+    remove_text_limit :error_tracking_error_events, :description
+    add_text_limit :error_tracking_error_events, :description, 255
+  end
+end

db/post_migrate/20210825150212_cleanup_remaining_orphan_invites.rb

+# frozen_string_literal: true
+
+class CleanupRemainingOrphanInvites < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  TMP_INDEX_NAME = 'tmp_idx_members_with_orphaned_invites'
+
+  QUERY_CONDITION = "invite_token IS NOT NULL AND user_id IS NOT NULL"
+
+  def up
+    membership = define_batchable_model('members')
+
+    add_concurrent_index :members, :id, where: QUERY_CONDITION, name: TMP_INDEX_NAME
+
+    membership.where(QUERY_CONDITION).pluck(:id).each_slice(10) do |group|
+      membership.where(id: group).where(QUERY_CONDITION).update_all(invite_token: nil)
+    end
+
+    remove_concurrent_index_by_name :members, TMP_INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :members, TMP_INDEX_NAME if index_exists_by_name?(:members, TMP_INDEX_NAME)
+  end
+end

db/schema_migrations/20210825104558

+ab678fb5e8ddf7e6dc84f36248440e94953d7c85ee6a50f4e5c06f32c6ee66ec
\ No newline at end of file

db/schema_migrations/20210825150212

+5dc6a4f9ecbd705bf8361c65b29931cde94968084e8ae7945a27acdcbd6475c8
\ No newline at end of file

db/structure.sql

@@ -12929,7 +12929,7 @@ CREATE TABLE error_tracking_error_events (
     payload jsonb DEFAULT '{}'::jsonb NOT NULL,
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
-    CONSTRAINT check_92ecc3077b CHECK ((char_length(description) <= 255)),
+    CONSTRAINT check_92ecc3077b CHECK ((char_length(description) <= 1024)),
     CONSTRAINT check_c67d5b8007 CHECK ((char_length(level) <= 255)),
     CONSTRAINT check_f4b52474ad CHECK ((char_length(environment) <= 255))
 );

spec/frontend/content_editor/extensions/attachment_spec.js

 import axios from 'axios';
 import MockAdapter from 'axios-mock-adapter';
-import { once } from 'lodash';
-import waitForPromises from 'helpers/wait_for_promises';
 import Attachment from '~/content_editor/extensions/attachment';
 import Image from '~/content_editor/extensions/image';
 import Link from '~/content_editor/extensions/link';
@@ -20,7 +18,6 @@ const PROJECT_WIKI_ATTACHMENT_LINK_HTML = `<p data-sourcepos="1:1-1:26" dir="aut
 
 describe('content_editor/extensions/attachment', () => {
   let tiptapEditor;
-  let eq;
   let doc;
   let p;
   let image;
@@ -33,6 +30,24 @@ describe('content_editor/extensions/attachment', () => {
   const imageFile = new File(['foo'], 'test-file.png', { type: 'image/png' });
   const attachmentFile = new File(['foo'], 'test-file.zip', { type: 'application/zip' });
 
+  const expectDocumentAfterTransaction = ({ number, expectedDoc, action }) => {
+    return new Promise((resolve) => {
+      let counter = 1;
+      const handleTransaction = () => {
+        if (counter === number) {
+          expect(tiptapEditor.state.doc.toJSON()).toEqual(expectedDoc.toJSON());
+          tiptapEditor.off('update', handleTransaction);
+          resolve();
+        }
+
+        counter += 1;
+      };
+
+      tiptapEditor.on('update', handleTransaction);
+      action();
+    });
+  };
+
   beforeEach(() => {
     renderMarkdown = jest.fn();
 
@@ -42,7 +57,6 @@ describe('content_editor/extensions/attachment', () => {
 
     ({
       builders: { doc, p, image, loading, link },
-      eq,
     } = createDocBuilder({
       tiptapEditor,
       names: {
@@ -98,18 +112,14 @@ describe('content_editor/extensions/attachment', () => {
           mock.onPost().reply(httpStatus.OK, successResponse);
         });
 
-        it('inserts an image with src set to the encoded image file and uploading true', (done) => {
+        it('inserts an image with src set to the encoded image file and uploading true', async () => {
           const expectedDoc = doc(p(image({ uploading: true, src: base64EncodedFile })));
 
-          tiptapEditor.on(
-            'update',
-            once(() => {
-              expect(eq(tiptapEditor.state.doc, expectedDoc)).toBe(true);
-              done();
-            }),
-          );
-
-          tiptapEditor.commands.uploadAttachment({ file: imageFile });
+          await expectDocumentAfterTransaction({
+            number: 1,
+            expectedDoc,
+            action: () => tiptapEditor.commands.uploadAttachment({ file: imageFile }),
+          });
         });
 
         it('updates the inserted image with canonicalSrc when upload is successful', async () => {
@@ -124,11 +134,11 @@ describe('content_editor/extensions/attachment', () => {
             ),
           );
 
-          tiptapEditor.commands.uploadAttachment({ file: imageFile });
-
-          await waitForPromises();
-
-          expect(eq(tiptapEditor.state.doc, expectedDoc)).toBe(true);
+          await expectDocumentAfterTransaction({
+            number: 2,
+            expectedDoc,
+            action: () => tiptapEditor.commands.uploadAttachment({ file: imageFile }),
+          });
         });
       });
 
@@ -137,14 +147,14 @@ describe('content_editor/extensions/attachment', () => {
           mock.onPost().reply(httpStatus.INTERNAL_SERVER_ERROR);
         });
 
-        it('resets the doc to orginal state', async () => {
+        it('resets the doc to original state', async () => {
           const expectedDoc = doc(p(''));
 
-          tiptapEditor.commands.uploadAttachment({ file: imageFile });
-
-          await waitForPromises();
-
-          expect(eq(tiptapEditor.state.doc, expectedDoc)).toBe(true);
+          await expectDocumentAfterTransaction({
+            number: 2,
+            expectedDoc,
+            action: () => tiptapEditor.commands.uploadAttachment({ file: imageFile }),
+          });
         });
 
         it('emits an error event that includes an error message', (done) => {
@@ -176,18 +186,14 @@ describe('content_editor/extensions/attachment', () => {
           mock.onPost().reply(httpStatus.OK, successResponse);
         });
 
-        it('inserts a loading mark', (done) => {
+        it('inserts a loading mark', async () => {
           const expectedDoc = doc(p(loading({ label: 'test-file' })));
 
-          tiptapEditor.on(
-            'update',
-            once(() => {
-              expect(eq(tiptapEditor.state.doc, expectedDoc)).toBe(true);
-              done();
-            }),
-          );
-
-          tiptapEditor.commands.uploadAttachment({ file: attachmentFile });
+          await expectDocumentAfterTransaction({
+            number: 1,
+            expectedDoc,
+            action: () => tiptapEditor.commands.uploadAttachment({ file: attachmentFile }),
+          });
         });
 
         it('updates the loading mark with a link with canonicalSrc and href attrs', async () => {
@@ -204,11 +210,11 @@ describe('content_editor/extensions/attachment', () => {
             ),
           );
 
-          tiptapEditor.commands.uploadAttachment({ file: attachmentFile });
-
-          await waitForPromises();
-
-          expect(eq(tiptapEditor.state.doc, expectedDoc)).toBe(true);
+          await expectDocumentAfterTransaction({
+            number: 2,
+            expectedDoc,
+            action: () => tiptapEditor.commands.uploadAttachment({ file: attachmentFile }),
+          });
         });
       });
 
@@ -220,11 +226,11 @@ describe('content_editor/extensions/attachment', () => {
         it('resets the doc to orginal state', async () => {
           const expectedDoc = doc(p(''));
 
-          tiptapEditor.commands.uploadAttachment({ file: attachmentFile });
-
-          await waitForPromises();
-
-          expect(eq(tiptapEditor.state.doc, expectedDoc)).toBe(true);
+          await expectDocumentAfterTransaction({
+            number: 2,
+            expectedDoc,
+            action: () => tiptapEditor.commands.uploadAttachment({ file: attachmentFile }),
+          });
         });
 
         it('emits an error event that includes an error message', (done) => {

spec/frontend/popovers/components/popovers_spec.js

@@ -54,17 +54,20 @@ describe('popovers/components/popovers.vue', () => {
       expect(wrapper.findAll(GlPopover)).toHaveLength(1);
     });
 
-    it('supports HTML content', async () => {
-      const content = 'content with <b>HTML</b>';
-      await buildWrapper(
-        createPopoverTarget({
-          content,
-          html: true,
-        }),
-      );
-      const html = wrapper.find(GlPopover).html();
-
-      expect(html).toContain(content);
+    describe('supports HTML content', () => {
+      const svgIcon = '<svg><use xlink:href="icons.svg#test"></use></svg>';
+
+      it.each`
+        description                         | content                          | render
+        ${'renders html content correctly'} | ${'<b>HTML</b>'}                 | ${'<b>HTML</b>'}
+        ${'removes any unsafe content'}     | ${'<script>alert(XSS)</script>'} | ${''}
+        ${'renders svg icons correctly'}    | ${svgIcon}                       | ${svgIcon}
+      `('$description', async ({ content, render }) => {
+        await buildWrapper(createPopoverTarget({ content, html: true }));
+
+        const html = wrapper.find(GlPopover).html();
+        expect(html).toContain(render);
+      });
     });
 
     it.each`

spec/migrations/cleanup_remaining_orphan_invites_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration! 'cleanup_remaining_orphan_invites'
+
+RSpec.describe CleanupRemainingOrphanInvites, :migration do
+  def create_member(**extra_attributes)
+    defaults = {
+      access_level: 10,
+      source_id: 1,
+      source_type: "Project",
+      notification_level: 0,
+      type: 'ProjectMember'
+    }
+
+    table(:members).create!(defaults.merge(extra_attributes))
+  end
+
+  def create_user(**extra_attributes)
+    defaults = { projects_limit: 0 }
+    table(:users).create!(defaults.merge(extra_attributes))
+  end
+
+  describe '#up', :aggregate_failures do
+    it 'removes invite tokens for accepted records' do
+      record1 = create_member(invite_token: 'foo', user_id: nil)
+      record2 = create_member(invite_token: 'foo2', user_id: create_user(username: 'foo', email: 'foo@example.com').id)
+      record3 = create_member(invite_token: nil, user_id: create_user(username: 'bar', email: 'bar@example.com').id)
+
+      migrate!
+
+      expect(table(:members).find(record1.id).invite_token).to eq 'foo'
+      expect(table(:members).find(record2.id).invite_token).to eq nil
+      expect(table(:members).find(record3.id).invite_token).to eq nil
+    end
+  end
+end

spec/models/error_tracking/error_spec.rb

@@ -16,6 +16,24 @@ RSpec.describe ErrorTracking::Error, type: :model do
     it { is_expected.to validate_presence_of(:actor) }
   end
 
+  describe '.report_error' do
+    it 'updates existing record with a new timestamp' do
+      timestamp = Time.zone.now
+
+      reported_error = described_class.report_error(
+        name: error.name,
+        description: 'Lorem ipsum',
+        actor: error.actor,
+        platform: error.platform,
+        timestamp: timestamp
+      )
+
+      expect(reported_error.id).to eq(error.id)
+      expect(reported_error.last_seen_at).to eq(timestamp)
+      expect(reported_error.description).to eq('Lorem ipsum')
+    end
+  end
+
   describe '#title' do
     it { expect(error.title).to eq('ActionView::MissingTemplate Missing template posts/edit') }
   end

spec/services/error_tracking/collect_error_service_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe ErrorTracking::CollectErrorService do
       expect(error.platform).to eq 'ruby'
       expect(error.last_seen_at).to eq '2021-07-08T12:59:16Z'
 
-      expect(event.description).to eq 'ActionView::MissingTemplate'
+      expect(event.description).to start_with 'Missing template posts/error2'
       expect(event.occurred_at).to eq '2021-07-08T12:59:16Z'
       expect(event.level).to eq 'error'
       expect(event.environment).to eq 'development'

spec/services/merge_requests/mergeability_check_service_spec.rb

@@ -132,6 +132,15 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar
 
     it_behaves_like 'mergeable merge request'
 
+    it 'calls MergeToRefService with cache parameter' do
+      service = instance_double(MergeRequests::MergeToRefService)
+
+      expect(MergeRequests::MergeToRefService).to receive(:new).once { service }
+      expect(service).to receive(:execute).once.with(merge_request, true).and_return(success: true)
+
+      described_class.new(merge_request).execute(recheck: true)
+    end
+
     context 'when concurrent calls' do
       it 'waits first lock and returns "cached" result in subsequent calls' do
         threads = execute_within_threads(amount: 3)