Add latest changes from gitlab-org/gitlab@master

3 files changed, 186 insertions, 0 deletions

diff --git a/spec/lib/gitlab/background_migration/migrate_users_bio_to_user_details_spec.rb b/spec/lib/gitlab/background_migration/migrate_users_bio_to_user_details_spec.rb
new file mode 100644
index 00000000000..8603eb73bd5
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/migrate_users_bio_to_user_details_spec.rb
@@ -0,0 +1,102 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::MigrateUsersBioToUserDetails, :migration, schema: 20200323074147 do
+  let(:users) { table(:users) }
+
+  let(:user_details) do
+    klass = table(:user_details)
+    klass.primary_key = :user_id
+    klass
+  end
+
+  let!(:user_needs_migration) { users.create(name: 'user1', email: 'test1@test.com', projects_limit: 1, bio: 'bio') }
+  let!(:user_needs_no_migration) { users.create(name: 'user2', email: 'test2@test.com', projects_limit: 1) }
+  let!(:user_also_needs_no_migration) { users.create(name: 'user3', email: 'test3@test.com', projects_limit: 1, bio: '') }
+  let!(:user_with_long_bio) { users.create(name: 'user4', email: 'test4@test.com', projects_limit: 1, bio: 'a' * 256) } # 255 is the max
+
+  let!(:user_already_has_details) { users.create(name: 'user5', email: 'test5@test.com', projects_limit: 1, bio: 'my bio') }
+  let!(:existing_user_details) { user_details.find_or_create_by(user_id: user_already_has_details.id).update(bio: 'my bio') }
+
+  # unlikely scenario since we have triggers
+  let!(:user_has_different_details) { users.create(name: 'user6', email: 'test6@test.com', projects_limit: 1, bio: 'different') }
+  let!(:different_existing_user_details) { user_details.find_or_create_by(user_id: user_has_different_details.id).update(bio: 'bio') }
+
+  let(:user_ids) do
+    [
+      user_needs_migration,
+      user_needs_no_migration,
+      user_also_needs_no_migration,
+      user_with_long_bio,
+      user_already_has_details,
+      user_has_different_details
+    ].map(&:id)
+  end
+
+  subject { described_class.new.perform(user_ids.min, user_ids.max) }
+
+  it 'migrates all relevant records' do
+    subject
+
+    all_user_details = user_details.all
+    expect(all_user_details.size).to eq(4)
+  end
+
+  it 'migrates `bio`' do
+    subject
+
+    user_detail = user_details.find_by!(user_id: user_needs_migration.id)
+
+    expect(user_detail.bio).to eq('bio')
+  end
+
+  it 'migrates long `bio`' do
+    subject
+
+    user_detail = user_details.find_by!(user_id: user_with_long_bio.id)
+
+    expect(user_detail.bio).to eq('a' * 255)
+  end
+
+  it 'does not change existing user detail' do
+    expect { subject }.not_to change { user_details.find_by!(user_id: user_already_has_details.id).attributes }
+  end
+
+  it 'changes existing user detail when the columns are different' do
+    expect { subject }.to change { user_details.find_by!(user_id: user_has_different_details.id).bio }.from('bio').to('different')
+  end
+
+  it 'does not migrate record' do
+    subject
+
+    user_detail = user_details.find_by(user_id: user_needs_no_migration.id)
+
+    expect(user_detail).to be_nil
+  end
+
+  it 'does not migrate empty bio' do
+    subject
+
+    user_detail = user_details.find_by(user_id: user_also_needs_no_migration.id)
+
+    expect(user_detail).to be_nil
+  end
+
+  context 'when `migrate_bio_to_user_details` feature flag is off' do
+    before do
+      stub_feature_flags(migrate_bio_to_user_details: false)
+    end
+
+    it 'does nothing' do
+      already_existing_user_details = user_details.where(user_id: [
+        user_has_different_details.id,
+        user_already_has_details.id
+      ])
+
+      subject
+
+      expect(user_details.all).to match_array(already_existing_user_details)
+    end
+  end
+end
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index 9ac2660908c..8b765ce122d 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -657,6 +657,30 @@ describe Gitlab::Database::MigrationHelpers do
         end
       end
 
+      context 'when `update_column_in_batches_args` is given' do
+        let(:column) { UserDetail.columns.find { |c| c.name == "user_id" } }
+
+        it 'uses `user_id` for `update_column_in_batches`' do
+          allow(model).to receive(:transaction_open?).and_return(false)
+          allow(model).to receive(:transaction).and_yield
+          allow(model).to receive(:column_for).with(:user_details, :foo).and_return(column)
+          allow(model).to receive(:update_column_in_batches).with(:user_details, :foo, 10, batch_column_name: :user_id)
+          allow(model).to receive(:change_column_null).with(:user_details, :foo, false)
+          allow(model).to receive(:change_column_default).with(:user_details, :foo, 10)
+
+          expect(model).to receive(:add_column)
+            .with(:user_details, :foo, :integer, default: nil)
+
+          model.add_column_with_default(
+            :user_details,
+            :foo,
+            :integer,
+            default: 10,
+            update_column_in_batches_args: { batch_column_name: :user_id }
+          )
+        end
+      end
+
       context 'when a column limit is set' do
         it 'adds the column with a limit' do
           allow(model).to receive(:transaction_open?).and_return(false)
diff --git a/spec/lib/gitlab/import_export/members_mapper_spec.rb b/spec/lib/gitlab/import_export/members_mapper_spec.rb
index 7e2b5ed534f..61e893bfb3c 100644
--- a/spec/lib/gitlab/import_export/members_mapper_spec.rb
+++ b/spec/lib/gitlab/import_export/members_mapper_spec.rb
@@ -78,6 +78,66 @@ describe Gitlab::ImportExport::MembersMapper do
         members_mapper.map
       end
 
+      context 'logging' do
+        let(:logger) { Gitlab::Import::Logger.build }
+
+        before do
+          allow(logger).to receive(:info)
+          allow(members_mapper).to receive(:logger).and_return(logger)
+        end
+
+        it 'logs member addition' do
+          expected_log_params = ->(user_id) do
+            {
+              user_id: user_id,
+              root_namespace_id: importable.root_ancestor.id,
+              importable_type: importable.class.to_s,
+              importable_id: importable.id,
+              access_level: exported_members.first['access_level'],
+              message: '[Project/Group Import] Added new member'
+            }
+          end
+
+          expect(logger).to receive(:info).with(hash_including(expected_log_params.call(user2.id))).once
+          expect(logger).to receive(:info).with(hash_including(expected_log_params.call(nil))).once
+
+          members_mapper.map
+        end
+
+        context 'when exporter member is invalid' do
+          let(:exported_members) do
+            [
+              {
+                "id" => 2,
+                "access_level" => -5, # invalid access level
+                "source_id" => 14,
+                "source_type" => source_type,
+                "notification_level" => 3,
+                "created_at" => "2016-03-11T10:21:44.822Z",
+                "updated_at" => "2016-03-11T10:21:44.822Z",
+                "created_by_id" => nil,
+                "invite_email" => nil,
+                "invite_token" => nil,
+                "invite_accepted_at" => nil,
+                "user" =>
+                  {
+                    "id" => exported_user_id,
+                    "email" => user2.email,
+                    "username" => 'test'
+                  },
+                "user_id" => 19
+              }
+            ]
+          end
+
+          it 'logs member addition failure' do
+            expect(logger).to receive(:info).with(hash_including(message: a_string_including('Access level is not included in the list'))).once
+
+            members_mapper.map
+          end
+        end
+      end
+
       context 'user is not an admin' do
         let(:user) { create(:user) }