Merge branch 'georgekoltsov/add-group-import-usage-ping' into 'master'

Add group importer usage ping

See merge request gitlab-org/gitlab!41663

app/models/group_import_state.rb

@@ -4,8 +4,9 @@ class GroupImportState < ApplicationRecord
   self.primary_key = :group_id
 
   belongs_to :group, inverse_of: :import_state
+  belongs_to :user, optional: false
 
-  validates :group, :status, presence: true
+  validates :group, :status, :user, presence: true
   validates :jid, presence: true, if: -> { started? || finished? }
 
   state_machine :status, initial: :created do

app/services/groups/import_export/import_service.rb

@@ -13,7 +13,7 @@ module Groups
       end
 
       def async_execute
-        group_import_state = GroupImportState.safe_find_or_create_by!(group: group)
+        group_import_state = GroupImportState.safe_find_or_create_by!(group: group, user: current_user)
         jid = GroupImportWorker.perform_async(current_user.id, group.id)
 
         if jid.present?

app/workers/group_import_worker.rb

@@ -9,7 +9,7 @@ class GroupImportWorker # rubocop:disable Scalability/IdempotentWorker
   def perform(user_id, group_id)
     current_user = User.find(user_id)
     group = Group.find(group_id)
-    group_import_state = group.import_state || group.build_import_state
+    group_import_state = group.import_state
 
     group_import_state.jid = self.jid
     group_import_state.start!

changelogs/unreleased/georgekoltsov-add-group-import-usage-ping.yml

+---
+title: Add Group Import usage ping
+merge_request: 41663
+author:
+type: added

db/migrate/20200907092610_add_user_id_to_group_import_states.rb

+# frozen_string_literal: true
+
+class AddUserIdToGroupImportStates < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    unless column_exists?(:group_import_states, :user_id)
+      with_lock_retries do
+        add_column :group_import_states, :user_id, :bigint
+      end
+    end
+
+    add_concurrent_foreign_key :group_import_states, :users, column: :user_id, on_delete: :cascade
+    add_concurrent_index :group_import_states, :user_id, where: 'user_id IS NOT NULL', name: 'index_group_import_states_on_user_id'
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :group_import_states, :user_id
+    end
+  end
+end

db/post_migrate/20200907092715_add_not_null_constraint_to_user_on_group_import_states.rb

+# frozen_string_literal: true
+
+class AddNotNullConstraintToUserOnGroupImportStates < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_not_null_constraint :group_import_states, :user_id, validate: false
+  end
+
+  def down
+    remove_not_null_constraint :group_import_states, :user_id
+  end
+end

db/post_migrate/20200909161624_cleanup_group_import_states_with_null_user_id.rb

+# frozen_string_literal: true
+
+class CleanupGroupImportStatesWithNullUserId < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # With BATCH_SIZE=1000 and group_import_states.count=600 on GitLab.com
+  # - 1 iteration will be run
+  # - each batch requires on average ~2500ms
+  # - 600 rows require on average ~1500ms
+  # Expected total run time: ~2500ms
+  BATCH_SIZE = 1000
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    self.table_name = 'users'
+  end
+
+  class Namespace < ActiveRecord::Base
+    self.table_name = 'namespaces'
+
+    belongs_to :parent, class_name: 'CleanupGroupImportStatesWithNullUserId::Namespace'
+    belongs_to :owner, class_name: 'CleanupGroupImportStatesWithNullUserId::User'
+  end
+
+  class Member < ActiveRecord::Base
+    self.table_name = 'members'
+    self.inheritance_column = :_type_disabled
+
+    belongs_to :user, class_name: 'CleanupGroupImportStatesWithNullUserId::User'
+  end
+
+  class Group < Namespace
+    OWNER = 50
+
+    self.inheritance_column = :_type_disabled
+
+    def default_owner
+      owners.first || parent&.default_owner || owner
+    end
+
+    def owners
+      Member.where(type: 'GroupMember', source_type: 'Namespace', source_id: id, requested_at: nil, access_level: OWNER).map(&:user)
+    end
+  end
+
+  class GroupImportState < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'group_import_states'
+
+    belongs_to :group, class_name: 'CleanupGroupImportStatesWithNullUserId::Group'
+    belongs_to :user, class_name: 'CleanupGroupImportStatesWithNullUserId::User'
+  end
+
+  def up
+    User.reset_column_information
+    Namespace.reset_column_information
+    Member.reset_column_information
+    Group.reset_column_information
+    GroupImportState.reset_column_information
+
+    GroupImportState.each_batch(of: BATCH_SIZE) do |batch|
+      batch.each do |group_import_state|
+        owner_id = Group.find_by_id(group_import_state.group_id)&.default_owner&.id
+
+        group_import_state.update!(user_id: owner_id) if owner_id
+      end
+    end
+
+    GroupImportState.where('user_id IS NULL').delete_all
+  end
+
+  def down
+    # no-op : can't go back to `NULL` without first dropping the `NOT NULL` constraint
+  end
+end

db/schema_migrations/20200907092610

+de1051e8f2d9f042ac923686b8c61e743c695736e8c7ebfdf6f87ab1ed09a11f
\ No newline at end of file

db/schema_migrations/20200907092715

+2610104c89134b94d460a714f17c23e5e35d76147c1f25be6c02804a39725d6c
\ No newline at end of file

db/schema_migrations/20200909161624

+2d7f514429e9a08ce13995feff43e221b3e2e74737ed48f81e104008d8ec24b9
\ No newline at end of file

db/structure.sql

@@ -12488,6 +12488,7 @@ CREATE TABLE group_import_states (
     status smallint DEFAULT 0 NOT NULL,
     jid text,
     last_error text,
+    user_id bigint,
     CONSTRAINT check_87b58f6b30 CHECK ((char_length(last_error) <= 255)),
     CONSTRAINT check_96558fff96 CHECK ((char_length(jid) <= 100))
 );
@@ -18085,6 +18086,9 @@ ALTER TABLE vulnerability_scanners
 ALTER TABLE packages_package_files
     ADD CONSTRAINT check_4c5e6bb0b3 CHECK ((file_store IS NOT NULL)) NOT VALID;
 
+ALTER TABLE group_import_states
+    ADD CONSTRAINT check_cda75c7c3f CHECK ((user_id IS NOT NULL)) NOT VALID;
+
 ALTER TABLE ONLY ci_build_needs
     ADD CONSTRAINT ci_build_needs_pkey PRIMARY KEY (id);
 
@@ -20219,6 +20223,8 @@ CREATE INDEX index_group_group_links_on_shared_with_group_id ON group_group_link
 
 CREATE INDEX index_group_import_states_on_group_id ON group_import_states USING btree (group_id);
 
+CREATE INDEX index_group_import_states_on_user_id ON group_import_states USING btree (user_id) WHERE (user_id IS NOT NULL);
+
 CREATE UNIQUE INDEX index_group_stages_on_group_id_group_value_stream_id_and_name ON analytics_cycle_analytics_group_stages USING btree (group_id, group_value_stream_id, name);
 
 CREATE UNIQUE INDEX index_group_wiki_repositories_on_disk_path ON group_wiki_repositories USING btree (disk_path);
@@ -22117,6 +22123,9 @@ ALTER TABLE ONLY merge_requests
 ALTER TABLE ONLY merge_request_metrics
     ADD CONSTRAINT fk_7f28d925f3 FOREIGN KEY (merged_by_id) REFERENCES users(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY group_import_states
+    ADD CONSTRAINT fk_8053b3ebd6 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY sprints
     ADD CONSTRAINT fk_80aa8a1f95 FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE;
 

lib/gitlab/usage_data.rb

@@ -557,7 +557,8 @@ module Gitlab
             jira: distinct_count(::JiraImportState.where(time_period), :user_id),
             fogbugz: projects_imported_count('fogbugz', time_period),
             phabricator: projects_imported_count('phabricator', time_period)
-          }
+          },
+          groups_imported: distinct_count(::GroupImportState.where(time_period), :user_id)
         }
       end
       # rubocop: enable CodeReuse/ActiveRecord

spec/factories/group_import_states.rb

@@ -3,6 +3,7 @@
 FactoryBot.define do
   factory :group_import_state, class: 'GroupImportState', traits: %i[created] do
     association :group, factory: :group
+    association :user, factory: :user
 
     trait :created do
       status { 0 }

spec/lib/gitlab/usage_data_spec.rb

@@ -255,6 +255,20 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
       )
     end
 
+    it 'includes group imports usage data' do
+      for_defined_days_back do
+        user = create(:user)
+        group = create(:group)
+        group.add_owner(user)
+        create(:group_import_state, group: group, user: user)
+      end
+
+      expect(described_class.usage_activity_by_stage_manage({}))
+        .to include(groups_imported: 2)
+      expect(described_class.usage_activity_by_stage_manage(described_class.last_28_days_time_period))
+        .to include(groups_imported: 1)
+    end
+
     def omniauth_providers
       [
         OpenStruct.new(name: 'google_oauth2'),

spec/migrations/cleanup_group_import_states_with_null_user_id_spec.rb

+# frozen_string_literal: true
+
+# In order to test the CleanupGroupImportStatesWithNullUserId migration, we need
+#  to first create GroupImportState with NULL user_id
+#  and then run the migration to check that user_id was populated or record removed
+#
+# The problem is that the CleanupGroupImportStatesWithNullUserId migration comes
+#  after the NOT NULL constraint has been added with a previous migration (AddNotNullConstraintToUserOnGroupImportStates)
+# That means that while testing the current class we can not insert GroupImportState records with an
+#  invalid user_id as constraint is blocking it from doing so
+#
+# To solve this problem, use SchemaVersionFinder to set schema one version prior to AddNotNullConstraintToUserOnGroupImportStates
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200907092715_add_not_null_constraint_to_user_on_group_import_states.rb')
+require Rails.root.join('db', 'post_migrate', '20200909161624_cleanup_group_import_states_with_null_user_id.rb')
+
+RSpec.describe CleanupGroupImportStatesWithNullUserId, :migration,
+               schema: MigrationHelpers::SchemaVersionFinder.migration_prior(AddNotNullConstraintToUserOnGroupImportStates) do
+  let(:namespaces_table) { table(:namespaces) }
+  let(:users_table) { table(:users) }
+  let(:group_import_states_table) { table(:group_import_states) }
+  let(:members_table) { table(:members) }
+
+  describe 'Group import states clean up' do
+    context 'when user_id is present' do
+      it 'does not update group_import_state record' do
+        user_1 = users_table.create!(name: 'user1', email: 'user1@example.com', projects_limit: 1)
+        group_1 = namespaces_table.create!(name: 'group_1', path: 'group_1', type: 'Group')
+        create_member(user_id: user_1.id, type: 'GroupMember', source_type: 'Namespace', source_id: group_1.id, access_level: described_class::Group::OWNER)
+        group_import_state_1 = group_import_states_table.create!(group_id: group_1.id, user_id: user_1.id, status: 0)
+
+        expect(group_import_state_1.user_id).to eq(user_1.id)
+
+        disable_migrations_output { migrate! }
+
+        expect(group_import_state_1.reload.user_id).to eq(user_1.id)
+      end
+    end
+
+    context 'when user_id is missing' do
+      it 'updates user_id with group default owner id' do
+        user_2 = users_table.create!(name: 'user2', email: 'user2@example.com', projects_limit: 1)
+        group_2 = namespaces_table.create!(name: 'group_2', path: 'group_2', type: 'Group')
+        create_member(user_id: user_2.id, type: 'GroupMember', source_type: 'Namespace', source_id: group_2.id, access_level: described_class::Group::OWNER)
+        group_import_state_2 = group_import_states_table.create!(group_id: group_2.id, user_id: nil, status: 0)
+
+        disable_migrations_output { migrate! }
+
+        expect(group_import_state_2.reload.user_id).to eq(user_2.id)
+      end
+    end
+
+    context 'when group does not contain any owners' do
+      it 'removes group_import_state record' do
+        group_3 = namespaces_table.create!(name: 'group_3', path: 'group_3', type: 'Group')
+        group_import_state_3 = group_import_states_table.create!(group_id: group_3.id, user_id: nil, status: 0)
+
+        disable_migrations_output { migrate! }
+
+        expect { group_import_state_3.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+  end
+
+  def create_member(options)
+    members_table.create!(
+      {
+        notification_level: 0,
+        ldap: false,
+        override: false
+      }.merge(options)
+    )
+  end
+end

spec/models/group_import_state_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe GroupImportState do
   describe 'validations' do
     let_it_be(:group) { create(:group) }
 
+    it { is_expected.to belong_to(:user).required }
     it { is_expected.to validate_presence_of(:group) }
     it { is_expected.to validate_presence_of(:status) }
 

spec/services/groups/import_export/import_service_spec.rb

@@ -10,6 +10,15 @@ RSpec.describe Groups::ImportExport::ImportService do
     context 'when the job can be successfully scheduled' do
       subject(:import_service) { described_class.new(group: group, user: user) }
 
+      it 'creates group import state' do
+        import_service.async_execute
+
+        import_state = group.import_state
+
+        expect(import_state.user).to eq(user)
+        expect(import_state.group).to eq(group)
+      end
+
       it 'enqueues an import job' do
         expect(GroupImportWorker).to receive(:perform_async).with(user.id, group.id)
 

spec/support/migrations_helpers/schema_version_finder.rb

+# frozen_string_literal: true
+
+# Sometimes data migration specs require adding invalid test data in order to test
+# the migration (e.g. adding a row with null foreign key). Certain db migrations that
+# add constraints (e.g. NOT NULL constraint) prevent invalid records from being added
+# and data migration from being tested. For this reason, SchemaVersionFinder can be used
+# to find and use schema prior to specified one.
+#
+# @example
+#   RSpec.describe CleanupThings, :migration, schema: MigrationHelpers::SchemaVersionFinder.migration_prior(AddNotNullConstraint) do ...
+#
+# SchemaVersionFinder returns schema version prior to the one specified, which allows to then add
+# invalid records to the database, which in return allows to properly test data migration.
+module MigrationHelpers
+  class SchemaVersionFinder
+    def self.migrations_paths
+      ActiveRecord::Migrator.migrations_paths
+    end
+
+    def self.migration_context
+      ActiveRecord::MigrationContext.new(migrations_paths, ActiveRecord::SchemaMigration)
+    end
+
+    def self.migrations
+      migration_context.migrations
+    end
+
+    def self.migration_prior(migration_klass)
+      migrations.each_cons(2) do |previous, migration|
+        break previous.version if migration.name == migration_klass.name
+      end
+    end
+  end
+end

spec/workers/group_import_worker_spec.rb

@@ -3,12 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe GroupImportWorker do
-  let!(:user) { create(:user) }
-  let!(:group) { create(:group) }
+  let(:user) { create(:user) }
+  let(:group) { create(:group) }
 
   subject { described_class.new }
 
   before do
+    create(:group_import_state, group: group, user: user)
+
     allow_next_instance_of(described_class) do |job|
       allow(job).to receive(:jid).and_return(SecureRandom.hex(8))
     end
@@ -26,44 +28,11 @@ RSpec.describe GroupImportWorker do
         subject.perform(user.id, group.id)
       end
 
-      context 'when the import state does not exist' do
-        it 'creates group import' do
-          expect(group.import_state).to be_nil
-
-          subject.perform(user.id, group.id)
-          import_state = group.reload.import_state
-
-          expect(import_state).to be_instance_of(GroupImportState)
-          expect(import_state.status_name).to eq(:finished)
-          expect(import_state.jid).not_to be_empty
-        end
-
-        it 'sets the group import status to started' do
-          expect_next_instance_of(GroupImportState) do |import|
-            expect(import).to receive(:start!).and_call_original
-          end
-
-          subject.perform(user.id, group.id)
-        end
-
-        it 'sets the group import status to finished' do
-          expect_next_instance_of(GroupImportState) do |import|
-            expect(import).to receive(:finish!).and_call_original
-          end
+      it 'updates the existing state' do
+        expect { subject.perform(user.id, group.id) }
+          .not_to change { GroupImportState.count }
 
-          subject.perform(user.id, group.id)
-        end
-      end
-
-      context 'when the import state already exists' do
-        it 'updates the existing state' do
-          existing_state = create(:group_import_state, group: group)
-
-          expect { subject.perform(user.id, group.id) }
-            .not_to change { GroupImportState.count }
-
-          expect(existing_state.reload).to be_finished
-        end
+        expect(group.import_state.reload).to be_finished
       end
     end
 
@@ -83,11 +52,9 @@ RSpec.describe GroupImportWorker do
         end
 
         it 'sets the group import status to failed' do
-          expect_next_instance_of(GroupImportState) do |import|
-            expect(import).to receive(:fail_op).and_call_original
-          end
-
           expect { subject.perform(user.id, group.id) }.to raise_exception(Gitlab::ImportExport::Error)
+
+          expect(group.import_state.reload.status).to eq(-1)
         end
       end
     end