Merge branch '273574-fix-bad-data-in-projects-has_external_issue_tracker' into 'master'

Fix bad data in projects.has_external_issue_tracker

See merge request gitlab-org/gitlab!53936

changelogs/unreleased/273574-fix-bad-projects-has_external_issue_tracker-data.yml

+---
+title: Cleanup incorrect data in projects.has_external_issue_tracker
+merge_request: 53936
+author:
+type: fixed

db/post_migrate/20210210221006_cleanup_projects_with_bad_has_external_issue_tracker_data.rb

+# frozen_string_literal: true
+
+class CleanupProjectsWithBadHasExternalIssueTrackerData < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  TMP_INDEX_NAME = 'tmp_idx_projects_on_id_where_has_external_issue_tracker_is_true'.freeze
+  BATCH_SIZE = 100
+
+  disable_ddl_transaction!
+
+  class Service < ActiveRecord::Base
+    include EachBatch
+    belongs_to :project
+
+    self.table_name = 'services'
+    self.inheritance_column = :_type_disabled
+  end
+
+  class Project < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'projects'
+  end
+
+  def up
+    update_projects_with_active_external_issue_trackers
+    update_projects_without_active_external_issue_trackers
+  end
+
+  def down
+    # no-op : can't go back to incorrect data
+  end
+
+  private
+
+  def update_projects_with_active_external_issue_trackers
+    scope = Service.where(active: true, category: 'issue_tracker').where.not(project_id: nil).distinct(:project_id)
+
+    scope.each_batch(of: BATCH_SIZE) do |relation|
+      scope_with_projects = relation
+        .joins(:project)
+        .select('project_id')
+        .merge(Project.where(has_external_issue_tracker: false).where(pending_delete: false))
+
+      execute(<<~SQL)
+      WITH project_ids_to_update (id) AS (
+        #{scope_with_projects.to_sql}
+      )
+      UPDATE projects SET has_external_issue_tracker = true WHERE id IN (SELECT id FROM project_ids_to_update)
+      SQL
+    end
+  end
+
+  def update_projects_without_active_external_issue_trackers
+    # Add a temporary index to speed up the scoping of projects.
+    index_where = <<~SQL
+      "projects"."has_external_issue_tracker" = TRUE
+      AND "projects"."pending_delete" = FALSE
+    SQL
+
+    add_concurrent_index(:projects, :id, where: index_where, name: TMP_INDEX_NAME)
+
+    services_sub_query = Service
+      .select('1')
+      .where('services.project_id = projects.id')
+      .where(category: 'issue_tracker')
+      .where(active: true)
+
+    # 322 projects are scoped in this query on GitLab.com.
+    Project.where(index_where).each_batch(of: BATCH_SIZE) do |relation|
+      relation_with_exists_query = relation.where('NOT EXISTS (?)', services_sub_query)
+      execute(<<~SQL)
+      WITH project_ids_to_update (id) AS (
+        #{relation_with_exists_query.select(:id).to_sql}
+      )
+      UPDATE projects SET has_external_issue_tracker = false WHERE id IN (SELECT id FROM project_ids_to_update)
+      SQL
+    end
+
+    # Drop the temporary index.
+    remove_concurrent_index_by_name(:projects, TMP_INDEX_NAME)
+  end
+end

db/schema_migrations/20210210221006

+1ff1256d2deac0a1545ef7db30d8ba7969265d6c2df62f6bd20f9f1721a482cb
\ No newline at end of file

spec/migrations/cleanup_projects_with_bad_has_external_issue_tracker_data_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require_migration!
+
+RSpec.describe CleanupProjectsWithBadHasExternalIssueTrackerData, :migration do
+  let(:namespace) { table(:namespaces).create!(name: 'foo', path: 'bar') }
+  let(:projects) { table(:projects) }
+  let(:services) { table(:services) }
+
+  def create_projects!(num)
+    Array.new(num) do
+      projects.create!(namespace_id: namespace.id)
+    end
+  end
+
+  def create_active_external_issue_tracker_integrations!(*projects)
+    projects.each do |project|
+      services.create!(category: 'issue_tracker', project_id: project.id, active: true)
+    end
+  end
+
+  def create_disabled_external_issue_tracker_integrations!(*projects)
+    projects.each do |project|
+      services.create!(category: 'issue_tracker', project_id: project.id, active: false)
+    end
+  end
+
+  def create_active_other_integrations!(*projects)
+    projects.each do |project|
+      services.create!(category: 'not_an_issue_tracker', project_id: project.id, active: true)
+    end
+  end
+
+  it 'sets `projects.has_external_issue_tracker` correctly' do
+    allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false)
+
+    project_with_an_external_issue_tracker_1,
+      project_with_an_external_issue_tracker_2,
+      project_with_only_a_disabled_external_issue_tracker_1,
+      project_with_only_a_disabled_external_issue_tracker_2,
+      project_without_any_external_issue_trackers_1,
+      project_without_any_external_issue_trackers_2 = create_projects!(6)
+
+    create_active_external_issue_tracker_integrations!(
+      project_with_an_external_issue_tracker_1,
+      project_with_an_external_issue_tracker_2
+    )
+
+    create_disabled_external_issue_tracker_integrations!(
+      project_with_an_external_issue_tracker_1,
+      project_with_an_external_issue_tracker_2,
+      project_with_only_a_disabled_external_issue_tracker_1,
+      project_with_only_a_disabled_external_issue_tracker_2
+    )
+
+    create_active_other_integrations!(
+      project_with_an_external_issue_tracker_1,
+      project_with_an_external_issue_tracker_2,
+      project_without_any_external_issue_trackers_1,
+      project_without_any_external_issue_trackers_2
+    )
+
+    # PG triggers on the services table added in
+    # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/51852 will have set
+    # the `has_external_issue_tracker` columns to correct data when the services
+    # records were created above.
+    #
+    # We set the `has_external_issue_tracker` columns for projects to incorrect
+    # data manually below to emulate projects in a state before the PG
+    # triggers were added.
+    project_with_an_external_issue_tracker_2.update!(has_external_issue_tracker: false)
+    project_with_only_a_disabled_external_issue_tracker_2.update!(has_external_issue_tracker: true)
+    project_without_any_external_issue_trackers_2.update!(has_external_issue_tracker: true)
+
+    migrate!
+
+    expected_true = [
+      project_with_an_external_issue_tracker_1,
+      project_with_an_external_issue_tracker_2
+    ].each(&:reload).map(&:has_external_issue_tracker)
+
+    expected_not_true = [
+      project_without_any_external_issue_trackers_1,
+      project_without_any_external_issue_trackers_2,
+      project_with_only_a_disabled_external_issue_tracker_1,
+      project_with_only_a_disabled_external_issue_tracker_2
+    ].each(&:reload).map(&:has_external_issue_tracker)
+
+    expect(expected_true).to all(eq(true))
+    expect(expected_not_true).to all(be_falsey)
+  end
+end