Automatic merge of gitlab-org/gitlab master

app/uploaders/object_storage.rb

@@ -283,8 +283,9 @@ module ObjectStorage
         if file_storage?
           IO.copy_stream(path, file)
         else
-          streamer = lambda { |chunk, _, _| file.write(chunk) }
-          Excon.get(url, response_block: streamer)
+          Faraday.get(url) do |req|
+            req.options.on_data = proc { |chunk, _| file.write(chunk) }
+          end
         end
 
         file.seek(0, IO::SEEK_SET)

lib/gitlab/github_import/importer/pull_request_review_importer.rb

@@ -69,8 +69,8 @@ module Gitlab
             author_id: author_id,
             note: note,
             system: false,
-            created_at: review.submitted_at,
-            updated_at: review.submitted_at
+            created_at: submitted_at,
+            updated_at: submitted_at
           }.merge(extra)
         end
 
@@ -80,8 +80,8 @@ module Gitlab
           approval_attribues = {
             merge_request_id: merge_request.id,
             user_id: user_id,
-            created_at: review.submitted_at,
-            updated_at: review.submitted_at
+            created_at: submitted_at,
+            updated_at: submitted_at
           }
 
           result = ::Approval.insert(
@@ -105,6 +105,10 @@ module Gitlab
 
           Note.create!(attributes)
         end
+
+        def submitted_at
+          @submitted_at ||= (review.submitted_at || merge_request.updated_at)
+        end
       end
     end
   end

lib/gitlab/github_import/representation/pull_request_review.rb

@@ -29,7 +29,7 @@ module Gitlab
           hash = Representation.symbolize_hash(raw_hash)
 
           hash[:author] &&= Representation::User.from_json_hash(hash[:author])
-          hash[:submitted_at] = Time.parse(hash[:submitted_at]).in_time_zone
+          hash[:submitted_at] = Time.parse(hash[:submitted_at]).in_time_zone if hash[:submitted_at].present?
 
           new(hash)
         end

spec/features/boards/boards_spec.rb

@@ -244,8 +244,7 @@ RSpec.describe 'Project issue boards', :js do
         expect(page).to have_selector(selector, text: development.title, count: 1)
       end
 
-      # TODO https://gitlab.com/gitlab-org/gitlab/-/issues/323551
-      xit 'issue moves between lists and does not show the "Development" label since the card is in the "Development" list label' do
+      it 'issue moves between lists and does not show the "Development" label since the card is in the "Development" list label' do
         drag(list_from_index: 1, from_index: 1, list_to_index: 2)
 
         wait_for_board_cards(2, 7)

spec/initializers/mailer_retries_spec.rb

@@ -2,22 +2,8 @@
 
 require 'spec_helper'
 
-RSpec.describe 'Mailer retries' do
-  # We need to ensure that this runs through Sidekiq to take
-  # advantage of the middleware. There is a Rails bug that means we
-  # have to do some extra steps to make this happen:
-  # https://github.com/rails/rails/issues/37270#issuecomment-553927324
-  around do |example|
-    descendants = ActiveJob::Base.descendants + [ActiveJob::Base]
-    descendants.each(&:disable_test_adapter)
-    ActiveJob::Base.queue_adapter = :sidekiq
-
-    example.run
-
-    descendants.each { |a| a.queue_adapter = :test }
-  end
-
-  it 'sets retries for mailers to 3', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/332645' do
+RSpec.describe 'Mailer retries', :sidekiq_mailers do
+  it 'sets retries for mailers to 3' do
     DeviseMailer.user_admin_approval(create(:user)).deliver_later
 
     expect(Sidekiq::Queues['mailers'].first).to include('retry' => 3)

spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb

@@ -167,6 +167,19 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
       end
     end
 
+    context 'when the submitted_at is not provided' do
+      let(:review) { create_review(type: 'APPROVED', note: '', submitted_at: nil) }
+
+      it 'creates a note for the review without the author information' do
+        expect { subject.execute }.to change(Note, :count).by(1)
+
+        last_note = merge_request.notes.last
+
+        expect(last_note.created_at)
+          .to be_within(1.second).of(merge_request.updated_at)
+      end
+    end
+
     context 'when the review has a note text' do
       context 'when the review is "APPROVED"' do
         let(:review) { create_review(type: 'APPROVED') }
@@ -215,13 +228,15 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
     end
   end
 
-  def create_review(type:, note: 'note', author: { id: 999, login: 'author' })
+  def create_review(type:, **extra)
     Gitlab::GithubImport::Representation::PullRequestReview.from_json_hash(
-      merge_request_id: merge_request.id,
-      review_type: type,
-      note: note,
-      submitted_at: submitted_at.to_s,
-      author: author
+      extra.reverse_merge(
+        author: { id: 999, login: 'author' },
+        merge_request_id: merge_request.id,
+        review_type: type,
+        note: 'note',
+        submitted_at: submitted_at.to_s
+      )
     )
   end
 end

spec/lib/gitlab/github_import/representation/pull_request_review_spec.rb

@@ -68,5 +68,11 @@ RSpec.describe Gitlab::GithubImport::Representation::PullRequestReview do
 
       expect(review.author).to be_nil
     end
+
+    it 'does not fail when submitted_at is blank' do
+      review = described_class.from_json_hash(hash.except('submitted_at'))
+
+      expect(review.submitted_at).to be_nil
+    end
   end
 end

spec/lib/marginalia_spec.rb

@@ -89,21 +89,7 @@ RSpec.describe 'Marginalia spec' do
       end
     end
 
-    describe 'for ActionMailer delivery jobs' do
-      # We need to ensure that this runs through Sidekiq to take
-      # advantage of the middleware. There is a Rails bug that means we
-      # have to do some extra steps to make this happen:
-      # https://github.com/rails/rails/issues/37270#issuecomment-553927324
-      around do |example|
-        descendants = ActiveJob::Base.descendants + [ActiveJob::Base]
-        descendants.each(&:disable_test_adapter)
-        ActiveJob::Base.queue_adapter = :sidekiq
-
-        example.run
-
-        descendants.each { |a| a.queue_adapter = :test }
-      end
-
+    describe 'for ActionMailer delivery jobs', :sidekiq_mailers do
       let(:delivery_job) { MarginaliaTestMailer.first_user.deliver_later }
 
       let(:recorded) do

spec/services/notification_service_spec.rb

@@ -929,6 +929,10 @@ RSpec.describe NotificationService, :mailer do
       end
 
       context 'design management is disabled' do
+        before do
+          enable_design_management(false)
+        end
+
         it 'does not notify anyone' do
           notification.new_note(note)
 

spec/support/sidekiq.rb

@@ -20,4 +20,25 @@ RSpec.configure do |config|
   config.around(:example, :sidekiq_inline) do |example|
     gitlab_sidekiq_inline { example.run }
   end
+
+  # Some specs need to run mailers through Sidekiq explicitly, rather
+  # than the ActiveJob test adapter. There is a Rails bug that means we
+  # have to do some extra steps to make this happen:
+  # https://github.com/rails/rails/issues/37270
+  #
+  # In particular, we can't use an `around` hook because then the 'before' part
+  # of that will run before the `before_setup` hook in ActiveJob::TestHelper,
+  # which doesn't do what we want.
+  #
+  config.before(:example, :sidekiq_mailers) do
+    queue_adapter_changed_jobs.each { |k| k.queue_adapter = :sidekiq }
+    queue_adapter_changed_jobs.each(&:disable_test_adapter)
+  end
+
+  config.after(:example, :sidekiq_mailers) do
+    queue_adapter_changed_jobs.each do |klass|
+      klass.queue_adapter = :test
+      klass.enable_test_adapter(ActiveJob::QueueAdapters::TestAdapter.new)
+    end
+  end
 end

spec/uploaders/object_storage_spec.rb

@@ -222,7 +222,11 @@ RSpec.describe ObjectStorage do
 
           before do
             stub_artifacts_object_storage
-            stub_request(:get, %r{s3.amazonaws.com/#{uploader.path}}).to_return(status: 200, body: '')
+
+            # We need to check the Host header not including the port because AWS does not accept
+            stub_request(:get, %r{s3.amazonaws.com/#{uploader.path}})
+              .with { |request| !request.headers['Host'].to_s.include?(':443') }
+              .to_return(status: 200, body: '')
           end
 
           it "returns the file" do