Merge branch 'cat-fix-sprite-failed-builds' into 'master'

Fix retried builds icon sprite to use css_class

See merge request gitlab-org/gitlab!46955

app/helpers/page_layout_helper.rb

 # frozen_string_literal: true
 
 module PageLayoutHelper
+  include Gitlab::Utils::StrongMemoize
+
   def page_title(*titles)
     @page_title ||= []
 
@@ -44,7 +46,7 @@ module PageLayoutHelper
     if link
       @page_canonical_link = link
     else
-      @page_canonical_link
+      @page_canonical_link ||= generic_canonical_url
     end
   end
 
@@ -147,4 +149,31 @@ module PageLayoutHelper
 
     css_class.join(' ')
   end
+
+  private
+
+  def generic_canonical_url
+    strong_memoize(:generic_canonical_url) do
+      next unless request.get? || request.head?
+      next unless generate_generic_canonical_url?
+      next unless Feature.enabled?(:generic_canonical, current_user)
+
+      # Request#url builds the url without the trailing slash
+      request.url
+    end
+  end
+
+  def generate_generic_canonical_url?
+    # For the main domain it doesn't matter whether there is
+    # a trailing slash or not, they're not considered different
+    # pages
+    return false if request.path == '/'
+
+    # We only need to generate the canonical url when the request has a trailing
+    # slash. In the request object, only the `original_fullpath` and
+    # `original_url` keep the slash if it's present. Both `path` and
+    # `fullpath` would return the path without the slash.
+    # Therefore, we need to process `original_fullpath`
+    request.original_fullpath.sub(request.path, '')[0] == '/'
+  end
 end

app/models/concerns/storage/legacy_namespace.rb

@@ -90,7 +90,7 @@ module Storage
     end
 
     def old_repository_storages
-      @old_repository_storage_paths ||= repository_storages
+      @old_repository_storage_paths ||= repository_storages(legacy_only: true)
     end
 
     def repository_storages(legacy_only: false)

changelogs/unreleased/36820-fj-add-generic-canonical-url.yml

+---
+title: Generate canonical url and remove trailing slash
+merge_request: 46435
+author:
+type: changed

changelogs/unreleased/Change-BitBucket-Pull-Request-Import-to-a-Batched-Process.yml

+---
+title: Add Batch Support for Importing Pull Requests from Bitbucket
+merge_request: 46696
+author: Simon Schrottner
+type: performance

changelogs/unreleased/sh-hashed-storage-no-destroy-namespace-dir.yml

+---
+title: Fix group destroy not working with Gitaly Cluster
+merge_request: 46934
+author:
+type: fixed

changelogs/unreleased/skip-importing-features-disabled-in-gitea.yml

+---
+title: Skip disabled features when importing a project from Gitea
+merge_request: 46800
+author: John Kristensen (@jerrykan)
+type: fixed

config/feature_flags/development/generic_canonical.yml

+---
+name: generic_canonical
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46435
+rollout_issue_url:
+type: development
+group: group::editor
+default_enabled: false

lib/bitbucket_server/client.rb

@@ -8,9 +8,9 @@ module BitbucketServer
       @connection = Connection.new(options)
     end
 
-    def pull_requests(project_key, repo)
+    def pull_requests(project_key, repo, page_offset: 0, limit: nil)
       path = "/projects/#{project_key}/repos/#{repo}/pull-requests?state=ALL"
-      get_collection(path, :pull_request)
+      get_collection(path, :pull_request, page_offset: page_offset, limit: limit)
     end
 
     def activities(project_key, repo, pull_request_id)

lib/gitlab/bitbucket_server_import/importer.rb

@@ -177,16 +177,24 @@ module Gitlab
       # on the remote server. Then we have to issue a `git fetch` to download these
       # branches.
       def import_pull_requests
-        log_info(stage: 'import_pull_requests', message: 'starting')
-        pull_requests = client.pull_requests(project_key, repository_slug).to_a
+        page = 0
+
+        log_info(stage: 'import_pull_requests', message: "starting")
+
+        loop do
+          log_debug(stage: 'import_pull_requests', message: "importing page #{page} and batch-size #{BATCH_SIZE} from #{page * BATCH_SIZE} to #{(page + 1) * BATCH_SIZE}")
+
+          pull_requests = client.pull_requests(project_key, repository_slug, page_offset: page, limit: BATCH_SIZE).to_a
+
+          break if pull_requests.empty?
 
           # Creating branches on the server and fetching the newly-created branches
-        # may take a number of network round-trips. Do this in batches so that we can
-        # avoid doing a git fetch for every new branch.
-        pull_requests.each_slice(BATCH_SIZE) do |batch|
-          restore_branches(batch) if recover_missing_commits
+          # may take a number of network round-trips. This used to be done in batches to
+          # avoid doing a git fetch for every new branch, as the whole process is now
+          # batched, we do not need to separately do this in batches.
+          restore_branches(pull_requests) if recover_missing_commits
 
-          batch.each do |pull_request|
+          pull_requests.each do |pull_request|
             if already_imported?(pull_request)
               log_info(stage: 'import_pull_requests', message: 'already imported', iid: pull_request.iid)
             else
@@ -201,6 +209,9 @@ module Gitlab
             backtrace = Gitlab::BacktraceCleaner.clean_backtrace(e.backtrace)
             errors << { type: :pull_request, iid: pull_request.iid, errors: e.message, backtrace: backtrace.join("\n"), raw_response: pull_request.raw }
           end
+
+          log_debug(stage: 'import_pull_requests', message: "finished page #{page} and batch-size #{BATCH_SIZE}")
+          page += 1
         end
       end
 
@@ -416,6 +427,10 @@ module Gitlab
         }
       end
 
+      def log_debug(details)
+        logger.debug(log_base_data.merge(details))
+      end
+
       def log_info(details)
         logger.info(log_base_data.merge(details))
       end

lib/gitlab/legacy_github_import/importer.rb

@@ -303,6 +303,8 @@ module Gitlab
         end
 
         imported!(resource_type)
+      rescue ::Octokit::NotFound => e
+        errors << { type: resource_type, errors: e.message }
       end
 
       def imported?(resource_type)

spec/features/canonical_link_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Canonical link' do
+  include Spec::Support::Helpers::Features::CanonicalLinkHelpers
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :public, namespace: user.namespace) }
+  let_it_be(:issue) { create(:issue, project: project) }
+
+  let_it_be(:issue_request) { issue_url(issue) }
+  let_it_be(:project_request) { project_url(project) }
+
+  before do
+    sign_in(user)
+  end
+
+  shared_examples 'shows canonical link' do
+    specify do
+      visit request_url
+
+      expect(page).to have_canonical_link(expected_url)
+    end
+  end
+
+  shared_examples 'does not show canonical link' do
+    specify do
+      visit request_url
+
+      expect(page).not_to have_any_canonical_links
+    end
+  end
+
+  it_behaves_like 'does not show canonical link' do
+    let(:request_url) { issue_request }
+  end
+
+  it_behaves_like 'shows canonical link' do
+    let(:request_url) { issue_request + '/' }
+    let(:expected_url) { issue_request }
+  end
+
+  it_behaves_like 'shows canonical link' do
+    let(:request_url) { project_issues_url(project) + "/?state=opened" }
+    let(:expected_url) { project_issues_url(project, state: 'opened') }
+  end
+
+  it_behaves_like 'does not show canonical link' do
+    let(:request_url) { project_request }
+  end
+
+  it_behaves_like 'shows canonical link' do
+    let(:request_url) { project_request + '/' }
+    let(:expected_url) { project_request }
+  end
+
+  it_behaves_like 'shows canonical link' do
+    let(:query_params) { '?foo=bar' }
+    let(:request_url) { project_request + "/#{query_params}" }
+    let(:expected_url) { project_request + query_params }
+  end
+
+  # Hard-coded canonical links
+
+  it_behaves_like 'shows canonical link' do
+    let(:request_url) { explore_root_path }
+    let(:expected_url) { explore_projects_url }
+  end
+
+  context 'when feature flag generic_canonical is disabled' do
+    before do
+      stub_feature_flags(generic_canonical: false)
+    end
+
+    it_behaves_like 'does not show canonical link' do
+      let(:request_url) { issue_request + '/' }
+    end
+
+    it_behaves_like 'does not show canonical link' do
+      let(:request_url) { project_request + '/' }
+    end
+  end
+end

spec/features/dashboard/root_explore_spec.rb

@@ -30,12 +30,4 @@ RSpec.describe 'Root explore' do
 
     include_examples 'shows public projects'
   end
-
-  it 'includes canonical link to explore projects url' do
-    visit explore_root_path
-
-    canonial_link = page.find("head link[rel='canonical']", visible: false)
-
-    expect(canonial_link[:href]).to eq explore_projects_url
-  end
 end

spec/helpers/page_layout_helper_spec.rb

@@ -137,4 +137,75 @@ RSpec.describe PageLayoutHelper do
       end
     end
   end
+
+  describe '#page_canonical_link' do
+    let(:user) { build(:user) }
+
+    subject { helper.page_canonical_link(link) }
+
+    before do
+      allow(helper).to receive(:current_user).and_return(user)
+    end
+
+    context 'when link is passed' do
+      let(:link) { 'https://gitlab.com' }
+
+      it 'stores and returns the link value' do
+        expect(subject).to eq link
+        expect(helper.page_canonical_link(nil)).to eq link
+      end
+    end
+
+    context 'when no link is provided' do
+      let(:link) { nil }
+      let(:request) { ActionDispatch::Request.new(env) }
+      let(:env) do
+        {
+          'ORIGINAL_FULLPATH' => '/foo/',
+          'PATH_INFO' => '/foo',
+          'HTTP_HOST' => 'test.host',
+          'REQUEST_METHOD' => method,
+          'rack.url_scheme' => 'http'
+        }
+      end
+
+      before do
+        allow(helper).to receive(:request).and_return(request)
+      end
+
+      shared_examples 'generates the canonical url using the params in the context' do
+        specify { expect(subject).to eq 'http://test.host/foo' }
+      end
+
+      shared_examples 'does not return a canonical url' do
+        specify { expect(subject).to be_nil }
+      end
+
+      it_behaves_like 'generates the canonical url using the params in the context' do
+        let(:method) { 'GET' }
+      end
+
+      it_behaves_like 'generates the canonical url using the params in the context' do
+        let(:method) { 'HEAD' }
+      end
+
+      it_behaves_like 'does not return a canonical url' do
+        let(:method) { 'POST' }
+      end
+
+      it_behaves_like 'does not return a canonical url' do
+        let(:method) { 'PUT' }
+      end
+
+      context 'when feature flag generic_canonical is disabled' do
+        let(:method) { 'GET' }
+
+        before do
+          stub_feature_flags(generic_canonical: false)
+        end
+
+        it_behaves_like 'does not return a canonical url'
+      end
+    end
+  end
 end

spec/lib/bitbucket_server/client_spec.rb

@@ -19,6 +19,15 @@ RSpec.describe BitbucketServer::Client do
 
       subject.pull_requests(project, repo_slug)
     end
+
+    it 'requests a collection with offset and limit' do
+      offset = 10
+      limit = 100
+
+      expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :pull_request, page_offset: offset, limit: limit)
+
+      subject.pull_requests(project, repo_slug, page_offset: offset, limit: limit)
+    end
   end
 
   describe '#activities' do

spec/lib/gitlab/bitbucket_server_import/importer_spec.rb

@@ -112,7 +112,7 @@ RSpec.describe Gitlab::BitbucketServerImport::Importer do
       allow(subject).to receive(:delete_temp_branches)
       allow(subject).to receive(:restore_branches)
 
-      allow(subject.client).to receive(:pull_requests).and_return([pull_request])
+      allow(subject.client).to receive(:pull_requests).and_return([pull_request], [])
     end
 
     # As we are using Caching with redis, it is best to clean the cache after each test run, else we need to wait for
@@ -499,7 +499,7 @@ RSpec.describe Gitlab::BitbucketServerImport::Importer do
 
       before do
         Gitlab::Cache::Import::Caching.set_add(subject.already_imported_cache_key, pull_request_already_imported.iid)
-        allow(subject.client).to receive(:pull_requests).and_return([pull_request_to_be_imported, pull_request_already_imported])
+        allow(subject.client).to receive(:pull_requests).and_return([pull_request_to_be_imported, pull_request_already_imported], [])
       end
 
       it 'only imports one Merge Request, as the other on is in the cache' do
@@ -535,7 +535,7 @@ RSpec.describe Gitlab::BitbucketServerImport::Importer do
         updated_at: Time.now,
         merged?: true)
 
-      expect(subject.client).to receive(:pull_requests).and_return([pull_request])
+      expect(subject.client).to receive(:pull_requests).and_return([pull_request], [])
       expect(subject.client).to receive(:activities).and_return([])
       expect(subject).to receive(:import_repository).twice
     end

spec/lib/gitlab/legacy_github_import/importer_spec.rb

@@ -52,7 +52,7 @@ RSpec.describe Gitlab::LegacyGithubImport::Importer do
       allow_any_instance_of(Octokit::Client).to receive(:milestones).and_return([milestone, milestone])
       allow_any_instance_of(Octokit::Client).to receive(:issues).and_return([issue1, issue2])
       allow_any_instance_of(Octokit::Client).to receive(:pull_requests).and_return([pull_request, pull_request])
-      allow_any_instance_of(Octokit::Client).to receive(:issues_comments).and_return([])
+      allow_any_instance_of(Octokit::Client).to receive(:issues_comments).and_raise(Octokit::NotFound)
       allow_any_instance_of(Octokit::Client).to receive(:pull_requests_comments).and_return([])
       allow_any_instance_of(Octokit::Client).to receive(:last_response).and_return(double(rels: { next: nil }))
       allow_any_instance_of(Octokit::Client).to receive(:releases).and_return([release1, release2])
@@ -169,6 +169,7 @@ RSpec.describe Gitlab::LegacyGithubImport::Importer do
         errors: [
           { type: :label, url: "#{api_root}/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title can't be blank, Title is invalid" },
           { type: :issue, url: "#{api_root}/repos/octocat/Hello-World/issues/1348", errors: "Validation failed: Title can't be blank" },
+          { type: :issues_comments, errors: 'Octokit::NotFound' },
           { type: :wiki, errors: "Gitlab::Git::CommandError" }
         ]
       }

spec/models/namespace_spec.rb

@@ -687,7 +687,7 @@ RSpec.describe Namespace do
       let!(:project) { create(:project_empty_repo, namespace: namespace) }
 
       it 'has no repositories base directories to remove' do
-        allow(GitlabShellWorker).to receive(:perform_in)
+        expect(GitlabShellWorker).not_to receive(:perform_in)
 
         expect(File.exist?(path_in_dir)).to be(false)