Merge branch '10-5-stable-prepare-rc8' into '10-5-stable'

Prepare 10.5 RC8 release

See merge request gitlab-org/gitlab-ce!17155

13 files changed, 244 insertions, 246 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 20534b8eed0..7312e704932 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -41,12 +41,41 @@ module Ci
 
     scope :unstarted, ->() { where(runner_id: nil) }
     scope :ignore_failures, ->() { where(allow_failure: false) }
+
+    # This convoluted mess is because we need to handle two cases of
+    # artifact files during the migration. And a simple OR clause
+    # makes it impossible to optimize.
+
+    # Instead we want to use UNION ALL and do two carefully
+    # constructed disjoint queries. But Rails cannot handle UNION or
+    # UNION ALL queries so we do the query in a subquery and wrap it
+    # in an otherwise redundant WHERE IN query (IN is fine for
+    # non-null columns).
+
+    # This should all be ripped out when the migration is finished and
+    # replaced with just the new storage to avoid the extra work.
+
     scope :with_artifacts, ->() do
-      where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)',
-        '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id'))
+      old = Ci::Build.select(:id).where(%q[artifacts_file <> ''])
+      new = Ci::Build.select(:id).where(%q[(artifacts_file IS NULL OR artifacts_file = '') AND EXISTS (?)],
+                                        Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id'))
+      where('ci_builds.id IN (? UNION ALL ?)', old, new)
     end
-    scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) }
-    scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) }
+
+    scope :with_artifacts_not_expired, ->() do
+      old = Ci::Build.select(:id).where(%q[artifacts_file <> '' AND (artifacts_expire_at IS NULL OR artifacts_expire_at > ?)], Time.now)
+      new = Ci::Build.select(:id).where(%q[(artifacts_file IS NULL OR artifacts_file = '') AND EXISTS (?)],
+                                        Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id AND (expire_at IS NULL OR expire_at > ?)', Time.now))
+      where('ci_builds.id IN (? UNION ALL ?)', old, new)
+    end
+
+    scope :with_expired_artifacts, ->() do
+      old = Ci::Build.select(:id).where(%q[artifacts_file <> '' AND artifacts_expire_at < ?], Time.now)
+      new = Ci::Build.select(:id).where(%q[(artifacts_file IS NULL OR artifacts_file = '') AND EXISTS (?)],
+                                        Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id AND expire_at < ?', Time.now))
+      where('ci_builds.id IN (? UNION ALL ?)', old, new)
+    end
+
     scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) }
     scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) }
     scope :ref_protected, -> { where(protected: true) }
diff --git a/app/models/external_issue.rb b/app/models/external_issue.rb
index 2aaba2e4c90..282fd7edcb7 100644
--- a/app/models/external_issue.rb
+++ b/app/models/external_issue.rb
@@ -39,7 +39,7 @@ class ExternalIssue
   end
 
   def to_reference(_from = nil, full: nil)
-    id
+    reference_link_text
   end
 
   def reference_link_text(from = nil)
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index ab6f8ea44a9..4b186d93772 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -134,7 +134,7 @@ module MergeRequests
     end
 
     def append_closes_description
-      return unless issue
+      return unless issue&.to_reference.present?
 
       closes_issue = "Closes #{issue.to_reference}"
 
@@ -163,7 +163,7 @@ module MergeRequests
       return if merge_request.title.present?
 
       if issue_iid.present?
-        merge_request.title = "Resolve #{issue_iid}"
+        merge_request.title = "Resolve #{issue.to_reference}"
         branch_title = source_branch.downcase.remove(issue_iid.downcase).titleize.humanize
         merge_request.title += " \"#{branch_title}\"" if branch_title.present?
       end
diff --git a/changelogs/unreleased/expired-ci-artifacts.yml b/changelogs/unreleased/expired-ci-artifacts.yml
new file mode 100644
index 00000000000..2fcbdb02f84
--- /dev/null
+++ b/changelogs/unreleased/expired-ci-artifacts.yml
@@ -0,0 +1,5 @@
+---
+title: Change SQL for expired artifacts to use new ci_job_artifacts.expire_at
+merge_request: 16578
+author:
+type: performance
diff --git a/changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml b/changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml
new file mode 100644
index 00000000000..fddfba94192
--- /dev/null
+++ b/changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml
@@ -0,0 +1,5 @@
+---
+title: Resolve PrepareUntrackedUploads PostgreSQL syntax error
+merge_request: 17019
+author:
+type: fixed
diff --git a/db/migrate/20180119160751_optimize_ci_job_artifacts.rb b/db/migrate/20180119160751_optimize_ci_job_artifacts.rb
new file mode 100644
index 00000000000..9b4340ed7b7
--- /dev/null
+++ b/db/migrate/20180119160751_optimize_ci_job_artifacts.rb
@@ -0,0 +1,23 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class OptimizeCiJobArtifacts < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    # job_id is just here to be a covering index for index only scans
+    # since we'll almost always be joining against ci_builds on job_id
+    add_concurrent_index(:ci_job_artifacts, [:expire_at, :job_id])
+    add_concurrent_index(:ci_builds, [:artifacts_expire_at], where: "artifacts_file <> ''")
+  end
+
+  def down
+    remove_concurrent_index(:ci_job_artifacts, [:expire_at, :job_id])
+    remove_concurrent_index(:ci_builds, [:artifacts_expire_at], where: "artifacts_file <> ''")
+  end
+end
diff --git a/db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb b/db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb
new file mode 100644
index 00000000000..e46e793d9d2
--- /dev/null
+++ b/db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb
@@ -0,0 +1,47 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class SchedulePopulateUntrackedUploadsIfNeeded < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  FOLLOW_UP_MIGRATION = 'PopulateUntrackedUploads'.freeze
+
+  class UntrackedFile < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'untracked_files_for_uploads'
+  end
+
+  def up
+    if table_exists?(:untracked_files_for_uploads)
+      process_or_remove_table
+    end
+  end
+
+  def down
+    # nothing
+  end
+
+  private
+
+  def process_or_remove_table
+    if UntrackedFile.all.empty?
+      drop_temp_table
+    else
+      schedule_populate_untracked_uploads_jobs
+    end
+  end
+
+  def drop_temp_table
+    drop_table(:untracked_files_for_uploads, if_exists: true)
+  end
+
+  def schedule_populate_untracked_uploads_jobs
+    say "Scheduling #{FOLLOW_UP_MIGRATION} background migration jobs since there are rows in untracked_files_for_uploads."
+
+    bulk_queue_background_migration_jobs_by_range(
+      UntrackedFile, FOLLOW_UP_MIGRATION)
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index d07a4c31618..6b43fc8403c 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180206200543) do
+ActiveRecord::Schema.define(version: 20180208183958) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -293,6 +293,7 @@ ActiveRecord::Schema.define(version: 20180206200543) do
     t.integer "failure_reason"
   end
 
+  add_index "ci_builds", ["artifacts_expire_at"], name: "index_ci_builds_on_artifacts_expire_at", where: "(artifacts_file <> ''::text)", using: :btree
   add_index "ci_builds", ["auto_canceled_by_id"], name: "index_ci_builds_on_auto_canceled_by_id", using: :btree
   add_index "ci_builds", ["commit_id", "stage_idx", "created_at"], name: "index_ci_builds_on_commit_id_and_stage_idx_and_created_at", using: :btree
   add_index "ci_builds", ["commit_id", "status", "type"], name: "index_ci_builds_on_commit_id_and_status_and_type", using: :btree
@@ -333,6 +334,7 @@ ActiveRecord::Schema.define(version: 20180206200543) do
     t.string "file"
   end
 
+  add_index "ci_job_artifacts", ["expire_at", "job_id"], name: "index_ci_job_artifacts_on_expire_at_and_job_id", using: :btree
   add_index "ci_job_artifacts", ["job_id", "file_type"], name: "index_ci_job_artifacts_on_job_id_and_file_type", unique: true, using: :btree
   add_index "ci_job_artifacts", ["project_id"], name: "index_ci_job_artifacts_on_project_id", using: :btree
 
diff --git a/doc/topics/autodevops/index.md b/doc/topics/autodevops/index.md
index 01bd925bd6f..5f5ba2b69bc 100644
--- a/doc/topics/autodevops/index.md
+++ b/doc/topics/autodevops/index.md
@@ -95,7 +95,9 @@ Auto Deploy, and Auto Monitoring will be silently skipped.
 
 The Auto DevOps base domain is required if you want to make use of [Auto
 Review Apps](#auto-review-apps) and [Auto Deploy](#auto-deploy). It is defined
-under the project's CI/CD settings while [enabling Auto DevOps](#enabling-auto-devops).
+either under the project's CI/CD settings while 
+[enabling Auto DevOps](#enabling-auto-devops) or in instance-wide settings in
+the CI/CD section.
 It can also be set at the project or group level as a variable, `AUTO_DEVOPS_DOMAIN`.
 
 A wildcard DNS A record matching the base domain is required, for example,
diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
index a7a1bbe1752..298de005b9b 100644
--- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb
+++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
@@ -43,7 +43,11 @@ module Gitlab
 
         store_untracked_file_paths
 
-        schedule_populate_untracked_uploads_jobs
+        if UntrackedFile.all.empty?
+          drop_temp_table
+        else
+          schedule_populate_untracked_uploads_jobs
+        end
       end
 
       private
@@ -92,7 +96,7 @@ module Gitlab
           end
         end
 
-        yield(paths)
+        yield(paths) if paths.any?
       end
 
       def build_find_command(search_dir)
@@ -165,6 +169,11 @@ module Gitlab
         bulk_queue_background_migration_jobs_by_range(
           UntrackedFile, FOLLOW_UP_MIGRATION)
       end
+
+      def drop_temp_table
+        UntrackedFile.connection.drop_table(:untracked_files_for_uploads,
+                                            if_exists: true)
+      end
     end
   end
 end
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 6ba599cdf83..f6ba3a581ca 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -180,8 +180,8 @@ FactoryBot.define do
 
     trait :artifacts do
       after(:create) do |build|
-        create(:ci_job_artifact, :archive, job: build)
-        create(:ci_job_artifact, :metadata, job: build)
+        create(:ci_job_artifact, :archive, job: build, expire_at: build.artifacts_expire_at)
+        create(:ci_job_artifact, :metadata, job: build, expire_at: build.artifacts_expire_at)
         build.reload
       end
     end
diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
index 370c2490b97..4c4c143afd6 100644
--- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
@@ -8,8 +8,6 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
 
   before do
     DatabaseCleaner.clean
-
-    drop_temp_table_if_exists
   end
 
   after do
@@ -23,57 +21,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
     end
   end
 
-  # E.g. The installation is in use at the time of migration, and someone has
-  # just uploaded a file
-  shared_examples 'does not add files in /uploads/tmp' do
-    let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
-
-    before do
-      FileUtils.mkdir(File.dirname(tmp_file))
-      FileUtils.touch(tmp_file)
-    end
-
-    after do
-      FileUtils.rm(tmp_file)
-    end
-
-    it 'does not add files from /uploads/tmp' do
-      described_class.new.perform
-
-      expect(untracked_files_for_uploads.count).to eq(5)
-    end
-  end
-
-  it 'ensures the untracked_files_for_uploads table exists' do
-    expect do
-      described_class.new.perform
-    end.to change { ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads) }.from(false).to(true)
-  end
-
-  it 'has a path field long enough for really long paths' do
-    described_class.new.perform
-
-    component = 'a' * 255
-
-    long_path = [
-      'uploads',
-      component, # project.full_path
-      component  # filename
-    ].flatten.join('/')
-
-    record = untracked_files_for_uploads.create!(path: long_path)
-    expect(record.reload.path.size).to eq(519)
-  end
-
-  context "test bulk insert with ON CONFLICT DO NOTHING or IGNORE" do
-    around do |example|
-      # If this is CI, we use Postgres 9.2 so this whole context should be
-      # skipped since we're unable to use ON CONFLICT DO NOTHING or IGNORE.
-      if described_class.new.send(:can_bulk_insert_and_ignore_duplicates?)
-        example.run
-      end
-    end
-
+  shared_examples 'prepares the untracked_files_for_uploads table' do
     context 'when files were uploaded before and after hashed storage was enabled' do
       let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) }
       let!(:user) { create(:user, :with_avatar) }
@@ -90,6 +38,21 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
         UploadService.new(project2, uploaded_file, FileUploader).execute
       end
 
+      it 'has a path field long enough for really long paths' do
+        described_class.new.perform
+
+        component = 'a' * 255
+
+        long_path = [
+          'uploads',
+          component, # project.full_path
+          component  # filename
+        ].flatten.join('/')
+
+        record = untracked_files_for_uploads.create!(path: long_path)
+        expect(record.reload.path.size).to eq(519)
+      end
+
       it 'adds unhashed files to the untracked_files_for_uploads table' do
         described_class.new.perform
 
@@ -130,91 +93,66 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
         end
       end
 
+      # E.g. The installation is in use at the time of migration, and someone has
+      # just uploaded a file
       context 'when there are files in /uploads/tmp' do
-        it_behaves_like 'does not add files in /uploads/tmp'
-      end
-    end
-  end
-
-  context 'test bulk insert without ON CONFLICT DO NOTHING or IGNORE' do
-    before do
-      # If this is CI, we use Postgres 9.2 so this stub has no effect.
-      #
-      # If this is being run on Postgres 9.5+ or MySQL, then this stub allows us
-      # to test the bulk insert functionality without ON CONFLICT DO NOTHING or
-      # IGNORE.
-      allow_any_instance_of(described_class).to receive(:postgresql_pre_9_5?).and_return(true)
-    end
-
-    context 'when files were uploaded before and after hashed storage was enabled' do
-      let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) }
-      let!(:user) { create(:user, :with_avatar) }
-      let!(:project1) { create(:project, :with_avatar) }
-      let(:project2) { create(:project) } # instantiate after enabling hashed_storage
-
-      before do
-        # Markdown upload before enabling hashed_storage
-        UploadService.new(project1, uploaded_file, FileUploader).execute
+        let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') }
 
-        stub_application_setting(hashed_storage_enabled: true)
-
-        # Markdown upload after enabling hashed_storage
-        UploadService.new(project2, uploaded_file, FileUploader).execute
-      end
-
-      it 'adds unhashed files to the untracked_files_for_uploads table' do
-        described_class.new.perform
-
-        expect(untracked_files_for_uploads.count).to eq(5)
-      end
-
-      it 'adds files with paths relative to CarrierWave.root' do
-        described_class.new.perform
-        untracked_files_for_uploads.all.each do |file|
-          expect(file.path.start_with?('uploads/')).to be_truthy
+        before do
+          FileUtils.mkdir(File.dirname(tmp_file))
+          FileUtils.touch(tmp_file)
         end
-      end
-
-      it 'does not add hashed files to the untracked_files_for_uploads table' do
-        described_class.new.perform
 
-        hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path
-        expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey
-      end
+        after do
+          FileUtils.rm(tmp_file)
+        end
 
-      it 'correctly schedules the follow-up background migration jobs' do
-        described_class.new.perform
+        it 'does not add files from /uploads/tmp' do
+          described_class.new.perform
 
-        expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5)
-        expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+          expect(untracked_files_for_uploads.count).to eq(5)
+        end
       end
 
-      # E.g. from a previous failed run of this background migration
-      context 'when there is existing data in untracked_files_for_uploads' do
-        before do
-          described_class.new.perform
-        end
+      context 'when the last batch size exactly matches the max batch size' do
+        it 'does not raise error' do
+          stub_const("#{described_class}::FIND_BATCH_SIZE", 5)
 
-        it 'does not error or produce duplicates of existing data' do
           expect do
             described_class.new.perform
-          end.not_to change { untracked_files_for_uploads.count }.from(5)
+          end.not_to raise_error
+
+          expect(untracked_files_for_uploads.count).to eq(5)
         end
       end
+    end
+  end
 
-      context 'when there are files in /uploads/tmp' do
-        it_behaves_like 'does not add files in /uploads/tmp'
-      end
+  # If running on Postgres 9.2 (like on CI), this whole context is skipped
+  # since we're unable to use ON CONFLICT DO NOTHING or IGNORE.
+  context "test bulk insert with ON CONFLICT DO NOTHING or IGNORE", if: described_class.new.send(:can_bulk_insert_and_ignore_duplicates?) do
+    it_behaves_like 'prepares the untracked_files_for_uploads table'
+  end
+
+  # If running on Postgres 9.2 (like on CI), the stubbed method has no effect.
+  #
+  # If running on Postgres 9.5+ or MySQL, then this context effectively tests
+  # the bulk insert functionality without ON CONFLICT DO NOTHING or IGNORE.
+  context 'test bulk insert without ON CONFLICT DO NOTHING or IGNORE' do
+    before do
+      allow_any_instance_of(described_class).to receive(:postgresql_pre_9_5?).and_return(true)
     end
+
+    it_behaves_like 'prepares the untracked_files_for_uploads table'
   end
 
   # Very new or lightly-used installations that are running this migration
   # may not have an upload directory because they have no uploads.
   context 'when no files were ever uploaded' do
-    it 'does not add to the untracked_files_for_uploads table (and does not raise error)' do
+    it 'deletes the `untracked_files_for_uploads` table (and does not raise error)' do
       described_class.new.perform
 
-      expect(untracked_files_for_uploads.count).to eq(0)
+      expect(untracked_files_for_uploads.connection.table_exists?(:untracked_files_for_uploads)).to be_falsey
     end
   end
 end
diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb
index a0d0a4fd81b..3a935d98540 100644
--- a/spec/services/merge_requests/build_service_spec.rb
+++ b/spec/services/merge_requests/build_service_spec.rb
@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 describe MergeRequests::BuildService do
+  using RSpec::Parameterized::TableSyntax
   include RepoHelpers
 
   let(:project) { create(:project, :repository) }
@@ -111,6 +112,7 @@ describe MergeRequests::BuildService do
 
     context 'one commit in the diff' do
       let(:commits) { Commit.decorate([commit_1], project) }
+      let(:commit_description) { commit_1.safe_message.split(/\n+/, 2).last }
 
       before do
         stub_compare
@@ -125,7 +127,7 @@ describe MergeRequests::BuildService do
       end
 
       it 'uses the description of the commit as the description of the merge request' do
-        expect(merge_request.description).to eq(commit_1.safe_message.split(/\n+/, 2).last)
+        expect(merge_request.description).to eq(commit_description)
       end
 
       context 'merge request already has a description set' do
@@ -148,68 +150,32 @@ describe MergeRequests::BuildService do
         end
       end
 
-      context 'branch starts with issue IID followed by a hyphen' do
-        let(:source_branch) { "#{issue.iid}-fix-issue" }
-
-        it 'appends "Closes #$issue-iid" to the description' do
-          expect(merge_request.description).to eq("#{commit_1.safe_message.split(/\n+/, 2).last}\n\nCloses ##{issue.iid}")
+      context 'when the source branch matches an issue' do
+        where(:issue_tracker, :source_branch, :closing_message) do
+          :jira                 | 'FOO-123-fix-issue' | 'Closes FOO-123'
+          :jira                 | 'fix-issue'         | nil
+          :custom_issue_tracker | '123-fix-issue'     | 'Closes #123'
+          :custom_issue_tracker | 'fix-issue'         | nil
+          :internal             | '123-fix-issue'     | 'Closes #123'
+          :internal             | 'fix-issue'         | nil
         end
 
-        context 'merge request already has a description set' do
-          let(:description) { 'Merge request description' }
-
-          it 'appends "Closes #$issue-iid" to the description' do
-            expect(merge_request.description).to eq("#{description}\n\nCloses ##{issue.iid}")
+        with_them do
+          before do
+            if issue_tracker == :internal
+              issue.update!(iid: 123)
+            else
+              create(:"#{issue_tracker}_service", project: project)
+            end
           end
-        end
 
-        context 'commit has no description' do
-          let(:commits) { Commit.decorate([commit_2], project) }
+          it 'appends the closing description' do
+            expected_description = [commit_description, closing_message].compact.join("\n\n")
 
-          it 'sets the description to "Closes #$issue-iid"' do
-            expect(merge_request.description).to eq("Closes ##{issue.iid}")
+            expect(merge_request.description).to eq(expected_description)
           end
         end
       end
-
-      context 'branch starts with numeric characters followed by a hyphen with no issue tracker' do
-        let(:source_branch) { '12345-fix-issue' }
-
-        before do
-          allow(project).to receive(:external_issue_tracker).and_return(false)
-          allow(project).to receive(:issues_enabled?).and_return(false)
-        end
-
-        it 'uses the title of the commit as the title of the merge request' do
-          expect(merge_request.title).to eq(commit_1.safe_message.split("\n").first)
-        end
-
-        it 'uses the description of the commit as the description of the merge request' do
-          commit_description = commit_1.safe_message.split(/\n+/, 2).last
-
-          expect(merge_request.description).to eq("#{commit_description}")
-        end
-      end
-
-      context 'branch starts with JIRA-formatted external issue IID followed by a hyphen' do
-        let(:source_branch) { 'EXMPL-12345-fix-issue' }
-
-        before do
-          allow(project).to receive(:external_issue_tracker).and_return(true)
-          allow(project).to receive(:issues_enabled?).and_return(false)
-          allow(project).to receive(:external_issue_reference_pattern).and_return(IssueTrackerService.reference_pattern)
-        end
-
-        it 'uses the title of the commit as the title of the merge request' do
-          expect(merge_request.title).to eq(commit_1.safe_message.split("\n").first)
-        end
-
-        it 'uses the description of the commit as the description of the merge request and appends the closes text' do
-          commit_description = commit_1.safe_message.split(/\n+/, 2).last
-
-          expect(merge_request.description).to eq("#{commit_description}\n\nCloses EXMPL-12345")
-        end
-      end
     end
 
     context 'more than one commit in the diff' do
@@ -239,90 +205,62 @@ describe MergeRequests::BuildService do
         end
       end
 
-      context 'branch starts with GitLab issue IID followed by a hyphen' do
-        let(:source_branch) { "#{issue.iid}-fix-issue" }
-
-        it 'sets the title to: Resolves "$issue-title"' do
-          expect(merge_request.title).to eq("Resolve \"#{issue.title}\"")
+      context 'when the source branch matches an issue' do
+        where(:issue_tracker, :source_branch, :title, :closing_message) do
+          :jira                 | 'FOO-123-fix-issue' | 'Resolve FOO-123 "Fix issue"' | 'Closes FOO-123'
+          :jira                 | 'fix-issue'         | 'Fix issue'                   | nil
+          :custom_issue_tracker | '123-fix-issue'     | 'Resolve #123 "Fix issue"'    | 'Closes #123'
+          :custom_issue_tracker | 'fix-issue'         | 'Fix issue'                   | nil
+          :internal             | '123-fix-issue'     | 'Resolve "A bug"'             | 'Closes #123'
+          :internal             | 'fix-issue'         | 'Fix issue'                   | nil
+          :internal             | '124-fix-issue'     | '124 fix issue'               | nil
         end
 
-        context 'when issue is not accessible to user' do
+        with_them do
           before do
-            project.team.truncate
-          end
-
-          it 'uses branch title as the merge request title' do
-            expect(merge_request.title).to eq("#{issue.iid} fix issue")
+            if issue_tracker == :internal
+              issue.update!(iid: 123)
+            else
+              create(:"#{issue_tracker}_service", project: project)
+            end
           end
-        end
-
-        context 'issue does not exist' do
-          let(:source_branch) { "#{issue.iid.succ}-fix-issue" }
 
-          it 'uses the title of the branch as the merge request title' do
-            expect(merge_request.title).to eq("#{issue.iid.succ} fix issue")
+          it 'sets the correct title' do
+            expect(merge_request.title).to eq(title)
           end
-        end
-
-        context 'issue is confidential' do
-          let(:issue_confidential) { true }
 
-          it 'uses the title of the branch as the merge request title' do
-            expect(merge_request.title).to eq("#{issue.iid} fix issue")
+          it 'sets the closing description' do
+            expect(merge_request.description).to eq(closing_message)
           end
         end
       end
 
-      context 'branch starts with numeric characters followed by a hyphen with no issue tracker' do
-        let(:source_branch) { '12345-fix-issue' }
+      context 'when the issue is not accessible to user' do
+        let(:source_branch) { "#{issue.iid}-fix-issue" }
 
         before do
-          allow(project).to receive(:external_issue_tracker).and_return(false)
-          allow(project).to receive(:issues_enabled?).and_return(false)
+          project.team.truncate
         end
 
-        it 'sets the title to the humanized branch title' do
-          expect(merge_request.title).to eq('12345 fix issue')
+        it 'uses branch title as the merge request title' do
+          expect(merge_request.title).to eq("#{issue.iid} fix issue")
         end
-      end
 
-      describe 'with JIRA enabled' do
-        before do
-          allow(project).to receive(:external_issue_tracker).and_return(true)
-          allow(project).to receive(:issues_enabled?).and_return(false)
-          allow(project).to receive(:external_issue_reference_pattern).and_return(IssueTrackerService.reference_pattern)
+        it 'does not set a description' do
+          expect(merge_request.description).to be_nil
         end
+      end
 
-        context 'branch does not start with JIRA-formatted external issue IID' do
-          let(:source_branch) { 'test-branch' }
+      context 'when the issue is confidential' do
+        let(:source_branch) { "#{issue.iid}-fix-issue" }
+        let(:issue_confidential) { true }
 
-          it 'sets the title to the humanized branch title' do
-            expect(merge_request.title).to eq('Test branch')
-          end
+        it 'uses the title of the branch as the merge request title' do
+          expect(merge_request.title).to eq("#{issue.iid} fix issue")
         end
 
-        context 'branch starts with JIRA-formatted external issue IID' do
-          let(:source_branch) { 'EXMPL-12345' }
-
-          it 'sets the title to the humanized branch title' do
-            expect(merge_request.title).to eq('Resolve EXMPL-12345')
-          end
-
-          it 'appends the closes text' do
-            expect(merge_request.description).to eq('Closes EXMPL-12345')
-          end
-
-          context 'followed by hyphenated text' do
-            let(:source_branch) { 'EXMPL-12345-fix-issue' }
-
-            it 'sets the title to the humanized branch title' do
-              expect(merge_request.title).to eq('Resolve EXMPL-12345 "Fix issue"')
-            end
-
-            it 'appends the closes text' do
-              expect(merge_request.description).to eq('Closes EXMPL-12345')
-            end
-          end
+        it 'does not set a description' do
+          expect(merge_request.description).to be_nil
         end
       end
     end