Add latest changes from gitlab-org/gitlab@master

8 files changed, 399 insertions, 87 deletions

diff --git a/spec/graphql/mutations/concerns/mutations/resolves_issuable_spec.rb b/spec/graphql/mutations/concerns/mutations/resolves_issuable_spec.rb
index 75b5d31d134..145e42e2a51 100644
--- a/spec/graphql/mutations/concerns/mutations/resolves_issuable_spec.rb
+++ b/spec/graphql/mutations/concerns/mutations/resolves_issuable_spec.rb
@@ -3,76 +3,27 @@
 require 'spec_helper'
 
 describe Mutations::ResolvesIssuable do
-  let(:mutation_class) do
+  let_it_be(:mutation_class) do
     Class.new(Mutations::BaseMutation) do
       include Mutations::ResolvesIssuable
     end
   end
 
-  let(:project)  { create(:project) }
-  let(:user)     { create(:user) }
-  let(:context)  { { current_user: user } }
-  let(:mutation) { mutation_class.new(object: nil, context: context, field: nil) }
-
-  shared_examples 'resolving an issuable' do |type|
-    context 'when user has access' do
-      let(:source) { type == :merge_request ? 'source_project' : 'project' }
-      let(:issuable) { create(type, author: user, "#{source}" => project) }
-
-      subject { mutation.resolve_issuable(type: type, parent_path: project.full_path, iid: issuable.iid) }
-
-      before do
-        project.add_developer(user)
-      end
-
-      it 'resolves issuable by iid' do
-        result = type == :merge_request ? subject.sync : subject
-        expect(result).to eq(issuable)
-      end
-
-      it 'uses the correct Resolver to resolve issuable' do
-        resolver_class = "Resolvers::#{type.to_s.classify.pluralize}Resolver".constantize
-        resolved_project = mutation.resolve_project(full_path: project.full_path)
-
-        allow(mutation).to receive(:resolve_project)
-          .with(full_path: project.full_path)
-          .and_return(resolved_project)
-
-        expect(resolver_class).to receive(:new)
-          .with(object: resolved_project, context: context, field: nil)
-          .and_call_original
-
-        subject
-      end
-
-      it 'uses the ResolvesProject to resolve project' do
-        expect(Resolvers::ProjectResolver).to receive(:new)
-          .with(object: nil, context: context, field: nil)
-          .and_call_original
-
-        subject
-      end
-
-      it 'returns nil if issuable is not found' do
-        result = mutation.resolve_issuable(type: type, parent_path: project.full_path, iid: "100")
-        result = type == :merge_request ? result.sync : result
-
-        expect(result).to be_nil
-      end
-
-      it 'returns nil if parent path is empty' do
-        result = mutation.resolve_issuable(type: type, parent_path: "", iid: issuable.iid)
-
-        expect(result).to be_nil
-      end
-    end
-  end
+  let_it_be(:project)  { create(:project) }
+  let_it_be(:user)     { create(:user) }
+  let_it_be(:context)  { { current_user: user } }
+  let_it_be(:mutation) { mutation_class.new(object: nil, context: context, field: nil) }
+  let(:parent) { issuable.project }
 
   context 'with issues' do
-    it_behaves_like 'resolving an issuable', :issue
+    let(:issuable) { create(:issue, project: project) }
+
+    it_behaves_like 'resolving an issuable in GraphQL', :issue
   end
 
   context 'with merge requests' do
-    it_behaves_like 'resolving an issuable', :merge_request
+    let(:issuable) { create(:merge_request, source_project: project) }
+
+    it_behaves_like 'resolving an issuable in GraphQL', :merge_request
   end
 end
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) }
 
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index ce3ee3fcfb0..13a0426624b 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -586,49 +586,97 @@ describe Member do
   end
 
   context 'when after_commit :update_highest_role' do
-    where(:member_type, :source_type) do
-      :project_member | :project
-      :group_member   | :group
-    end
+    context 'with feature flag enabled' do
+      where(:member_type, :source_type) do
+        :project_member | :project
+        :group_member   | :group
+      end
 
-    with_them do
-      describe 'create member' do
-        it 'initializes a new Members::UpdateHighestRoleService object' do
-          source = create(source_type) # source owner initializes a new service object too
-          user = create(:user)
+      with_them do
+        describe 'create member' do
+          it 'initializes a new Members::UpdateHighestRoleService object' do
+            source = create(source_type) # source owner initializes a new service object too
+            user = create(:user)
 
-          expect(Members::UpdateHighestRoleService).to receive(:new).with(user.id).and_call_original
+            expect(Members::UpdateHighestRoleService).to receive(:new).with(user.id).and_call_original
 
-          create(member_type, :guest, user: user, source_type => source)
+            create(member_type, :guest, user: user, source_type => source)
+          end
         end
-      end
 
-      context 'when member exists' do
-        let!(:member) { create(member_type) }
+        context 'when member exists' do
+          let!(:member) { create(member_type) }
+
+          describe 'update member' do
+            context 'when access level was changed' do
+              it 'initializes a new Members::UpdateHighestRoleService object' do
+                expect(Members::UpdateHighestRoleService).to receive(:new).with(member.user_id).and_call_original
+
+                member.update(access_level: Gitlab::Access::GUEST)
+              end
+            end
+
+            context 'when access level was not changed' do
+              it 'does not initialize a new Members::UpdateHighestRoleService object' do
+                expect(Members::UpdateHighestRoleService).not_to receive(:new).with(member.user_id)
+
+                member.update(notification_level: NotificationSetting.levels[:disabled])
+              end
+            end
+          end
 
-        describe 'update member' do
-          context 'when access level was changed' do
+          describe 'destroy member' do
             it 'initializes a new Members::UpdateHighestRoleService object' do
               expect(Members::UpdateHighestRoleService).to receive(:new).with(member.user_id).and_call_original
 
-              member.update(access_level: Gitlab::Access::GUEST)
+              member.destroy
             end
           end
+        end
+      end
+    end
 
-          context 'when access level was not changed' do
-            it 'does not initialize a new Members::UpdateHighestRoleService object' do
-              expect(Members::UpdateHighestRoleService).not_to receive(:new).with(member.user_id)
+    context 'with feature flag disabled' do
+      before do
+        stub_feature_flags(highest_role_callback: false)
+      end
 
-              member.update(notification_level: NotificationSetting.levels[:disabled])
-            end
+      where(:member_type, :source_type) do
+        :project_member | :project
+        :group_member   | :group
+      end
+
+      with_them do
+        describe 'create member' do
+          it 'does not initialize a new Members::UpdateHighestRoleService object' do
+            source = create(source_type)
+            user = create(:user)
+
+            expect(Members::UpdateHighestRoleService).not_to receive(:new).with(user.id)
+
+            create(member_type, :guest, user: user, source_type => source)
           end
         end
 
-        describe 'destroy member' do
-          it 'initializes a new Members::UpdateHighestRoleService object' do
-            expect(Members::UpdateHighestRoleService).to receive(:new).with(member.user_id).and_call_original
+        context 'when member exists' do
+          let!(:member) { create(member_type) }
 
-            member.destroy
+          describe 'update member' do
+            context 'when access level was changed' do
+              it 'does not initialize a new Members::UpdateHighestRoleService object' do
+                expect(Members::UpdateHighestRoleService).not_to receive(:new).with(member.user_id)
+
+                member.update(access_level: Gitlab::Access::GUEST)
+              end
+            end
+          end
+
+          describe 'destroy member' do
+            it 'does not initialize a new Members::UpdateHighestRoleService object' do
+              expect(Members::UpdateHighestRoleService).not_to receive(:new).with(member.user_id)
+
+              member.destroy
+            end
           end
         end
       end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index bbd45afadd7..b7abcb31321 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -55,6 +55,67 @@ describe User, :do_not_mock_admin_mode do
     it { is_expected.to have_many(:custom_attributes).class_name('UserCustomAttribute') }
     it { is_expected.to have_many(:releases).dependent(:nullify) }
 
+    describe "#bio" do
+      it 'syncs bio with `user_details.bio` on create' do
+        user = create(:user, bio: 'my bio')
+
+        expect(user.bio).to eq(user.user_detail.bio)
+      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 not sync bio with `user_details.bio`' do
+          user = create(:user, bio: 'my bio')
+
+          expect(user.bio).to eq('my bio')
+          expect(user.user_detail.bio).to eq('')
+        end
+      end
+
+      it 'syncs bio with `user_details.bio` on update' do
+        user = create(:user)
+
+        user.update!(bio: 'my bio')
+
+        expect(user.bio).to eq(user.user_detail.bio)
+      end
+
+      context 'when `user_details` association already exists' do
+        let(:user) { create(:user) }
+
+        before do
+          create(:user_detail, user: user)
+        end
+
+        it 'syncs bio with `user_details.bio`' do
+          user.update!(bio: 'my bio')
+
+          expect(user.bio).to eq(user.user_detail.bio)
+        end
+
+        it 'falls back to "" when nil is given' do
+          user.update!(bio: nil)
+
+          expect(user.bio).to eq(nil)
+          expect(user.user_detail.bio).to eq('')
+        end
+
+        # very unlikely scenario
+        it 'truncates long bio when syncing to user_details' do
+          invalid_bio = 'a' * 256
+          truncated_bio = 'a' * 255
+
+          user.bio = invalid_bio
+          user.save(validate: false)
+
+          expect(user.user_detail.bio).to eq(truncated_bio)
+        end
+      end
+    end
+
     describe "#abuse_report" do
       let(:current_user) { create(:user) }
       let(:other_user) { create(:user) }
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 6d1b76a9aea..92cc6dc887e 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -739,6 +739,17 @@ describe API::Users, :do_not_mock_admin_mode do
       expect(user.reload.bio).to eq('new test bio')
     end
 
+    it "updates user with empty bio" do
+      user.bio = 'previous bio'
+      user.save!
+
+      put api("/users/#{user.id}", admin), params: { bio: '' }
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response['bio']).to eq('')
+      expect(user.reload.bio).to eq('')
+    end
+
     it "updates user with new password and forces reset on next login" do
       put api("/users/#{user.id}", admin), params: { password: '12345678' }
 
diff --git a/spec/support/shared_examples/graphql/resolves_issuable_shared_examples.rb b/spec/support/shared_examples/graphql/resolves_issuable_shared_examples.rb
new file mode 100644
index 00000000000..b56181371c3
--- /dev/null
+++ b/spec/support/shared_examples/graphql/resolves_issuable_shared_examples.rb
@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+RSpec.shared_examples 'resolving an issuable in GraphQL' do |type|
+  subject { mutation.resolve_issuable(type: type, parent_path: parent.full_path, iid: issuable.iid) }
+
+  context 'when user has access' do
+    before do
+      parent.add_developer(user)
+    end
+
+    it 'resolves issuable by iid' do
+      result = type == :merge_request ? subject.sync : subject
+      expect(result).to eq(issuable)
+    end
+
+    it 'uses the correct Resolver to resolve issuable' do
+      resolver_class = "Resolvers::#{type.to_s.classify.pluralize}Resolver".constantize
+      resolve_method = type == :epic ? :resolve_group : :resolve_project
+      resolved_parent = mutation.send(resolve_method, full_path: parent.full_path)
+
+      allow(mutation).to receive(resolve_method)
+        .with(full_path: parent.full_path)
+        .and_return(resolved_parent)
+
+      expect(resolver_class).to receive(:new)
+        .with(object: resolved_parent, context: context, field: nil)
+        .and_call_original
+
+      subject
+    end
+
+    it 'uses correct Resolver to resolve issuable parent' do
+      resolver_class = type == :epic ? 'Resolvers::GroupResolver' : 'Resolvers::ProjectResolver'
+
+      expect(resolver_class.constantize).to receive(:new)
+        .with(object: nil, context: context, field: nil)
+        .and_call_original
+
+      subject
+    end
+
+    it 'returns nil if issuable is not found' do
+      result = mutation.resolve_issuable(type: type, parent_path: parent.full_path, iid: "100")
+      result = type == :merge_request ? result.sync : result
+
+      expect(result).to be_nil
+    end
+
+    it 'returns nil if parent path is not present' do
+      result = mutation.resolve_issuable(type: type, parent_path: "", iid: issuable.iid)
+
+      expect(result).to be_nil
+    end
+  end
+end