Merge branch 'kassio/fix-missing-author-notes-on-importers' into 'master'

Use the original author id to find it on the members mapper

See merge request gitlab-org/gitlab!42648

changelogs/unreleased/kassio-fix-missing-author-notes-on-importers.yml

+---
+title: Fixed a bug causing 'Missing author note' to be added to notes for mapped users when importing project using GitLab Import.
+merge_request: 42648
+author:
+type: fixed

ee/spec/lib/gitlab/import_export/group/relation_factory_spec.rb

@@ -5,16 +5,19 @@ require 'spec_helper'
 RSpec.describe Gitlab::ImportExport::Group::RelationFactory do
   let(:group) { create(:group) }
   let(:members_mapper) { double('members_mapper').as_null_object }
-  let(:user) { create(:admin) }
+  let(:admin) { create(:admin) }
+  let(:importer_user) { admin }
   let(:excluded_keys) { [] }
   let(:created_object) do
-    described_class.create(relation_sym:   relation_sym,
+    described_class.create(
+      relation_sym: relation_sym,
       relation_hash: relation_hash,
       members_mapper: members_mapper,
       object_builder: Gitlab::ImportExport::Group::ObjectBuilder,
-                           user:           user,
+      user: importer_user,
       importable: group,
-                           excluded_keys:  excluded_keys)
+      excluded_keys: excluded_keys
+    )
   end
 
   context 'epic object' do
@@ -65,27 +68,8 @@ RSpec.describe Gitlab::ImportExport::Group::RelationFactory do
     end
   end
 
-  context 'Notes user references' do
-    let(:relation_sym) { :notes }
-    let(:new_user) { create(:user) }
-    let(:exported_member) do
-      {
-        'id'                 => 111,
-        'access_level'       => 30,
-        'source_id'          => 1,
-        'source_type'        => 'Namespace',
-        'user_id'            => 3,
-        'notification_level' => 3,
-        'created_at'         => '2016-11-18T09:29:42.634Z',
-        'updated_at'         => '2016-11-18T09:29:42.634Z',
-        'user'               => {
-          'id'       => 999,
-          'email'    => new_user.email,
-          'username' => new_user.username
-        }
-      }
-    end
-
+  it_behaves_like 'Notes user references' do
+    let(:importable) { group }
     let(:relation_hash) do
       {
         'id' => 4947,
@@ -106,17 +90,6 @@ RSpec.describe Gitlab::ImportExport::Group::RelationFactory do
         'events' => []
       }
     end
-
-    let(:members_mapper) do
-      Gitlab::ImportExport::MembersMapper.new(
-        exported_members: [exported_member],
-        user:             user,
-        importable:       group)
-    end
-
-    it 'maps the right author to the imported note' do
-      expect(created_object.author).to eq(new_user)
-    end
   end
 
   def random_id

lib/gitlab/import_export/base/relation_factory.rb

@@ -53,6 +53,7 @@ module Gitlab
           @importable = importable
           @imported_object_retries = 0
           @relation_hash[importable_column_name] = @importable.id
+          @original_user = {}
 
           # Remove excluded keys from relation_hash
           # We don't do this in the parsed_relation_hash because of the 'transformed attributes'
@@ -112,6 +113,7 @@ module Gitlab
         def update_user_references
           self.class::USER_REFERENCES.each do |reference|
             if @relation_hash[reference]
+              @original_user[reference] = @relation_hash[reference]
               @relation_hash[reference] = @members_mapper.map[@relation_hash[reference]]
             end
           end
@@ -243,7 +245,7 @@ module Gitlab
         # will be used. Otherwise, a note stating the original author name
         # is left.
         def set_note_author
-          old_author_id = @relation_hash['author_id']
+          old_author_id = @original_user['author_id']
           author = @relation_hash.delete('author')
 
           update_note_for_missing_author(author['name']) unless has_author?(old_author_id)

lib/gitlab/import_export/members_mapper.rb

@@ -34,8 +34,8 @@ module Gitlab
         @user.id
       end
 
-      def include?(old_author_id)
-        map.has_key?(old_author_id) && map[old_author_id] != default_user_id
+      def include?(old_user_id)
+        map.has_key?(old_user_id) && map[old_user_id] != default_user_id
       end
 
       private

spec/lib/gitlab/import_export/group/relation_factory_spec.rb

@@ -5,16 +5,19 @@ require 'spec_helper'
 RSpec.describe Gitlab::ImportExport::Group::RelationFactory do
   let(:group) { create(:group) }
   let(:members_mapper) { double('members_mapper').as_null_object }
-  let(:user) { create(:admin) }
+  let(:admin) { create(:admin) }
+  let(:importer_user) { admin }
   let(:excluded_keys) { [] }
   let(:created_object) do
-    described_class.create(relation_sym:   relation_sym,
+    described_class.create(
+      relation_sym: relation_sym,
       relation_hash: relation_hash,
       members_mapper: members_mapper,
       object_builder: Gitlab::ImportExport::Group::ObjectBuilder,
-                           user:           user,
+      user: importer_user,
       importable: group,
-                           excluded_keys:  excluded_keys)
+      excluded_keys: excluded_keys
+    )
   end
 
   context 'label object' do
@@ -60,27 +63,8 @@ RSpec.describe Gitlab::ImportExport::Group::RelationFactory do
     end
   end
 
-  context 'Notes user references' do
-    let(:relation_sym) { :notes }
-    let(:new_user) { create(:user) }
-    let(:exported_member) do
-      {
-        'id'                 => 111,
-        'access_level'       => 30,
-        'source_id'          => 1,
-        'source_type'        => 'Namespace',
-        'user_id'            => 3,
-        'notification_level' => 3,
-        'created_at'         => '2016-11-18T09:29:42.634Z',
-        'updated_at'         => '2016-11-18T09:29:42.634Z',
-        'user'               => {
-          'id'       => 999,
-          'email'    => new_user.email,
-          'username' => new_user.username
-        }
-      }
-    end
-
+  it_behaves_like 'Notes user references' do
+    let(:importable) { group }
     let(:relation_hash) do
       {
         'id' => 4947,
@@ -101,17 +85,6 @@ RSpec.describe Gitlab::ImportExport::Group::RelationFactory do
         'events' => []
       }
     end
-
-    let(:members_mapper) do
-      Gitlab::ImportExport::MembersMapper.new(
-        exported_members: [exported_member],
-        user:             user,
-        importable:       group)
-    end
-
-    it 'maps the right author to the imported note' do
-      expect(created_object.author).to eq(new_user)
-    end
   end
 
   def random_id

spec/lib/gitlab/import_export/project/relation_factory_spec.rb

@@ -6,16 +6,19 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
   let(:group) { create(:group) }
   let(:project) { create(:project, :repository, group: group) }
   let(:members_mapper) { double('members_mapper').as_null_object }
-  let(:user) { create(:admin) }
+  let(:admin) { create(:admin) }
+  let(:importer_user) { admin }
   let(:excluded_keys) { [] }
   let(:created_object) do
-    described_class.create(relation_sym: relation_sym,
+    described_class.create(
+      relation_sym: relation_sym,
       relation_hash: relation_hash,
       object_builder: Gitlab::ImportExport::Project::ObjectBuilder,
       members_mapper: members_mapper,
-                           user: user,
+      user: importer_user,
       importable: project,
-                           excluded_keys: excluded_keys)
+      excluded_keys: excluded_keys
+    )
   end
 
   before do
@@ -113,9 +116,9 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
         "created_at" => "2016-11-18T09:29:42.634Z",
         "updated_at" => "2016-11-18T09:29:42.634Z",
         "user" => {
-          "id" => user.id,
-          "email" => user.email,
-          "username" => user.username
+          "id" => admin.id,
+          "email" => admin.email,
+          "username" => admin.username
         }
       }
     end
@@ -123,7 +126,7 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
     let(:members_mapper) do
       Gitlab::ImportExport::MembersMapper.new(
         exported_members: [exported_member],
-        user: user,
+        user: importer_user,
         importable: project)
     end
 
@@ -134,9 +137,9 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
         'source_branch' => "feature_conflict",
         'source_project_id' => project.id,
         'target_project_id' => project.id,
-        'author_id' => user.id,
-        'assignee_id' => user.id,
-        'updated_by_id' => user.id,
+        'author_id' => admin.id,
+        'assignee_id' => admin.id,
+        'updated_by_id' => admin.id,
         'title' => "MR1",
         'created_at' => "2016-06-14T15:02:36.568Z",
         'updated_at' => "2016-06-14T15:02:56.815Z",
@@ -151,11 +154,11 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
     end
 
     it 'has preloaded author' do
-      expect(created_object.author).to equal(user)
+      expect(created_object.author).to equal(admin)
     end
 
     it 'has preloaded updated_by' do
-      expect(created_object.updated_by).to equal(user)
+      expect(created_object.updated_by).to equal(admin)
     end
 
     it 'has preloaded source project' do
@@ -264,27 +267,8 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
     end
   end
 
-  context 'Notes user references' do
-    let(:relation_sym) { :notes }
-    let(:new_user) { create(:user) }
-    let(:exported_member) do
-      {
-        "id" => 111,
-        "access_level" => 30,
-        "source_id" => 1,
-        "source_type" => "Project",
-        "user_id" => 3,
-        "notification_level" => 3,
-        "created_at" => "2016-11-18T09:29:42.634Z",
-        "updated_at" => "2016-11-18T09:29:42.634Z",
-        "user" => {
-          "id" => 999,
-          "email" => new_user.email,
-          "username" => new_user.username
-        }
-      }
-    end
-
+  it_behaves_like 'Notes user references' do
+    let(:importable) { project }
     let(:relation_hash) do
       {
         "id" => 4947,
@@ -305,17 +289,6 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
         "events" => []
       }
     end
-
-    let(:members_mapper) do
-      Gitlab::ImportExport::MembersMapper.new(
-        exported_members: [exported_member],
-        user: user,
-        importable: project)
-    end
-
-    it 'maps the right author to the imported note' do
-      expect(created_object.author).to eq(new_user)
-    end
   end
 
   context 'encrypted attributes' do

spec/support/shared_examples/lib/gitlab/import_export/relation_factory_shared_examples.rb

+# frozen_string_literal: true
+
+# required context:
+# - importable: group or project
+# - relation_hash: a note relation that's being imported
+# - created_object: the object created with the relation factory
+RSpec.shared_examples 'Notes user references' do
+  let(:relation_sym) { :notes }
+  let(:mapped_user) { create(:user) }
+  let(:exported_member) do
+    {
+      'id' => 111,
+      'access_level' => 30,
+      'source_id' => 1,
+      'source_type' => importable.class.name == 'Project' ? 'Project' : 'Namespace',
+      'user_id' => 3,
+      'notification_level' => 3,
+      'created_at' => '2016-11-18T09:29:42.634Z',
+      'updated_at' => '2016-11-18T09:29:42.634Z',
+      'user' => {
+        'id' => 999,
+        'email' => mapped_user.email,
+        'username' => mapped_user.username
+      }
+    }
+  end
+
+  let(:members_mapper) do
+    Gitlab::ImportExport::MembersMapper.new(
+      exported_members: [exported_member].compact,
+      user: importer_user,
+      importable: importable
+    )
+  end
+
+  shared_examples 'sets the note author to the importer user' do
+    it { expect(created_object.author).to eq(importer_user) }
+  end
+
+  shared_examples 'sets the note author to the mapped user' do
+    it { expect(created_object.author).to eq(mapped_user) }
+  end
+
+  shared_examples 'does not add original autor note' do
+    it { expect(created_object.note).not_to include('*By Administrator') }
+  end
+
+  shared_examples 'adds original autor note' do
+    it { expect(created_object.note).to include('*By Administrator') }
+  end
+
+  context 'when the importer is admin' do
+    let(:importer_user) { create(:admin) }
+
+    context 'and the note author is not mapped' do
+      let(:exported_member) { nil }
+
+      include_examples 'sets the note author to the importer user'
+
+      include_examples 'adds original autor note'
+    end
+
+    context 'and the note author is the importer user' do
+      let(:mapped_user) { importer_user }
+
+      include_examples 'sets the note author to the mapped user'
+
+      include_examples 'adds original autor note'
+    end
+
+    context 'and the note author exists in the target instance' do
+      let(:mapped_user) { create(:user) }
+
+      include_examples 'sets the note author to the mapped user'
+
+      include_examples 'does not add original autor note'
+    end
+  end
+
+  context 'when the importer is not admin' do
+    let(:importer_user) { create(:user) }
+
+    context 'and the note author is not mapped' do
+      let(:exported_member) { nil }
+
+      include_examples 'sets the note author to the importer user'
+
+      include_examples 'adds original autor note'
+    end
+
+    context 'and the note author is the importer user' do
+      let(:mapped_user) { importer_user }
+
+      include_examples 'sets the note author to the importer user'
+
+      include_examples 'adds original autor note'
+    end
+
+    context 'and the note author exists in the target instance' do
+      let(:mapped_user) { create(:user) }
+
+      include_examples 'sets the note author to the importer user'
+
+      include_examples 'adds original autor note'
+    end
+  end
+end