From 17c099161ee582e627a531bda1d84574d6a8c0f7 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 18 Jan 2017 17:40:24 +0100
Subject: [PATCH] fix and refactor note user mapping

---
 lib/gitlab/import_export/members_mapper.rb    |  2 -
 lib/gitlab/import_export/relation_factory.rb  | 10 ++---
 .../import_export/members_mapper_spec.rb      | 43 ++++++++++++++++---
 .../import_export/relation_factory_spec.rb    |  4 +-
 4 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/lib/gitlab/import_export/members_mapper.rb b/lib/gitlab/import_export/members_mapper.rb
index 1dab7c37d25..ac7604d2461 100644
--- a/lib/gitlab/import_export/members_mapper.rb
+++ b/lib/gitlab/import_export/members_mapper.rb
@@ -7,7 +7,6 @@ module Gitlab
         @exported_members = user.admin? ? exported_members : []
         @user = user
         @project = project
-        @missing_author_ids = []
 
         # This needs to run first, as second call would be from #map
         # which means project members already exist.
@@ -39,7 +38,6 @@ module Gitlab
 
       def missing_keys_tracking_hash
         Hash.new do |_, key|
-          @missing_author_ids << key
           default_user_id
         end
       end
diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb
index a5f6fbbcfd3..e5f9aa39190 100644
--- a/lib/gitlab/import_export/relation_factory.rb
+++ b/lib/gitlab/import_export/relation_factory.rb
@@ -80,17 +80,13 @@ module Gitlab
       # is left.
       def set_note_author
         old_author_id = @relation_hash['author_id']
-
-        # Users with admin access can map users
-        @relation_hash['author_id'] = admin_user? ? @members_mapper.map[old_author_id] : @members_mapper.default_user_id
-
         author = @relation_hash.delete('author')
 
-        update_note_for_missing_author(author['name']) if missing_author?(old_author_id)
+        update_note_for_missing_author(author['name']) unless has_author?(old_author_id)
       end
 
-      def missing_author?(old_author_id)
-        !admin_user? || @members_mapper.missing_author_ids.include?(old_author_id)
+      def has_author?(old_author_id)
+        admin_user? && !@members_mapper.map.keys.include?(old_author_id)
       end
 
       def missing_author_note(updated_at, author_name)
diff --git a/spec/lib/gitlab/import_export/members_mapper_spec.rb b/spec/lib/gitlab/import_export/members_mapper_spec.rb
index af3a0ab2b45..39e6dad5abe 100644
--- a/spec/lib/gitlab/import_export/members_mapper_spec.rb
+++ b/spec/lib/gitlab/import_export/members_mapper_spec.rb
@@ -24,7 +24,7 @@ describe Gitlab::ImportExport::MembersMapper, services: true do
            {
              "id" => exported_user_id,
              "email" => user2.email,
-             "username" => user2.username
+             "username" => 'test'
            }
        },
        {
@@ -48,6 +48,12 @@ describe Gitlab::ImportExport::MembersMapper, services: true do
         exported_members: exported_members, user: user, project: project)
     end
 
+    it 'includes the exported user ID in the map' do
+      members_mapper.map[-1]
+
+      expect(members_mapper.map.keys).to include(exported_user_id)
+    end
+
     it 'maps a project member' do
       expect(members_mapper.map[exported_user_id]).to eq(user2.id)
     end
@@ -56,12 +62,6 @@ describe Gitlab::ImportExport::MembersMapper, services: true do
       expect(members_mapper.map[-1]).to eq(user.id)
     end
 
-    it 'updates missing author IDs on missing project member' do
-      members_mapper.map[-1]
-
-      expect(members_mapper.missing_author_ids.first).to eq(-1)
-    end
-
     it 'has invited members with no user' do
       members_mapper.map
 
@@ -86,5 +86,34 @@ describe Gitlab::ImportExport::MembersMapper, services: true do
         expect(members_mapper.map[-1]).to eq(user.id)
       end
     end
+
+    context 'chooses the one with an email first' do
+      before do
+        exported_members << {
+          "id" => 2,
+          "access_level" => 40,
+          "source_id" => 14,
+          "source_type" => "Project",
+          "user_id" => 19,
+          "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" => 'test@email.com',
+              "username" => user2.username
+            }
+        }
+      end
+
+      it 'maps the project member that has a matching email first' do
+        expect(members_mapper.map[exported_user_id]).to eq(user2.id)
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/import_export/relation_factory_spec.rb b/spec/lib/gitlab/import_export/relation_factory_spec.rb
index 4604e88b295..db0084d6823 100644
--- a/spec/lib/gitlab/import_export/relation_factory_spec.rb
+++ b/spec/lib/gitlab/import_export/relation_factory_spec.rb
@@ -128,7 +128,7 @@ describe Gitlab::ImportExport::RelationFactory, lib: true do
     let(:new_user) { create(:user) }
     let(:exported_member) do
       {
-        "id" => 999,
+        "id" => 111,
         "access_level" => 30,
         "source_id" => 1,
         "source_type" => "Project",
@@ -137,7 +137,7 @@ describe Gitlab::ImportExport::RelationFactory, lib: true do
         "created_at" => "2016-11-18T09:29:42.634Z",
         "updated_at" => "2016-11-18T09:29:42.634Z",
         "user" => {
-          "id" => new_user.id,
+          "id" => 999,
           "email" => new_user.email,
           "username" => new_user.username
         }