Merge branch 'bvl-link-phab-users' into 'master'

Fetch users from Phabricator + link to issues

Closes #60565

See merge request gitlab-org/gitlab-ce!30321

8 files changed, 272 insertions, 16 deletions

diff --git a/spec/lib/gitlab/phabricator_import/cache/map_spec.rb b/spec/lib/gitlab/phabricator_import/cache/map_spec.rb
index 52c7a02219f..b6629fad453 100644
--- a/spec/lib/gitlab/phabricator_import/cache/map_spec.rb
+++ b/spec/lib/gitlab/phabricator_import/cache/map_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 describe Gitlab::PhabricatorImport::Cache::Map, :clean_gitlab_redis_cache do
@@ -28,6 +30,21 @@ describe Gitlab::PhabricatorImport::Cache::Map, :clean_gitlab_redis_cache do
 
       expect(ttl).to be > 10.seconds
     end
+
+    it 'sets the object in redis once if a block was given and nothing was cached' do
+      issue = create(:issue, project: project)
+
+      expect(map.get_gitlab_model('does not exist') { issue }).to eq(issue)
+
+      expect { |b| map.get_gitlab_model('does not exist', &b) }
+        .not_to yield_control
+    end
+
+    it 'does not cache `nil` objects' do
+      expect(map).not_to receive(:set_gitlab_model)
+
+      map.get_gitlab_model('does not exist') { nil }
+    end
   end
 
   describe '#set_gitlab_model' do
diff --git a/spec/lib/gitlab/phabricator_import/conduit/user_spec.rb b/spec/lib/gitlab/phabricator_import/conduit/user_spec.rb
new file mode 100644
index 00000000000..e88eec2c393
--- /dev/null
+++ b/spec/lib/gitlab/phabricator_import/conduit/user_spec.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Gitlab::PhabricatorImport::Conduit::User do
+  let(:user_client) do
+    described_class.new(phabricator_url: 'https://see-ya-later.phabricator', api_token: 'api-token')
+  end
+
+  describe '#users' do
+    let(:fake_client) { double('Phabricator client') }
+
+    before do
+      allow(user_client).to receive(:client).and_return(fake_client)
+    end
+
+    it 'calls the api with the correct params' do
+      expected_params = {
+         constraints: { phids: ['phid-1', 'phid-2'] }
+      }
+
+      expect(fake_client).to receive(:get).with('user.search',
+                                                params: expected_params)
+
+      user_client.users(['phid-1', 'phid-2'])
+    end
+
+    it 'returns an array of parsed responses' do
+      response = Gitlab::PhabricatorImport::Conduit::Response
+                   .new(fixture_file('phabricator_responses/user.search.json'))
+
+      allow(fake_client).to receive(:get).and_return(response)
+
+      expect(user_client.users(%w[some phids])).to match_array([an_instance_of(Gitlab::PhabricatorImport::Conduit::UsersResponse)])
+    end
+
+    it 'performs multiple requests if more phids than the maximum page size are passed' do
+      stub_const('Gitlab::PhabricatorImport::Conduit::User::MAX_PAGE_SIZE', 1)
+      first_params = { constraints: { phids: ['phid-1'] } }
+      second_params = { constraints: { phids: ['phid-2'] } }
+
+      expect(fake_client).to receive(:get).with('user.search',
+                                                params: first_params).once
+      expect(fake_client).to receive(:get).with('user.search',
+                                                params: second_params).once
+
+      user_client.users(['phid-1', 'phid-2'])
+    end
+  end
+end
diff --git a/spec/lib/gitlab/phabricator_import/conduit/users_response_spec.rb b/spec/lib/gitlab/phabricator_import/conduit/users_response_spec.rb
new file mode 100644
index 00000000000..00778ad90fd
--- /dev/null
+++ b/spec/lib/gitlab/phabricator_import/conduit/users_response_spec.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Gitlab::PhabricatorImport::Conduit::UsersResponse do
+  let(:conduit_response) do
+    Gitlab::PhabricatorImport::Conduit::Response
+      .new(JSON.parse(fixture_file('phabricator_responses/user.search.json')))
+  end
+
+  subject(:response) { described_class.new(conduit_response) }
+
+  describe '#users' do
+    it 'builds the correct users representation' do
+      tasks = response.users
+
+      usernames = tasks.map(&:username)
+
+      expect(usernames).to contain_exactly('jane', 'john')
+    end
+  end
+end
diff --git a/spec/lib/gitlab/phabricator_import/issues/importer_spec.rb b/spec/lib/gitlab/phabricator_import/issues/importer_spec.rb
index 2412cf76f79..667321409da 100644
--- a/spec/lib/gitlab/phabricator_import/issues/importer_spec.rb
+++ b/spec/lib/gitlab/phabricator_import/issues/importer_spec.rb
@@ -2,7 +2,7 @@
 require 'spec_helper'
 
 describe Gitlab::PhabricatorImport::Issues::Importer do
-  set(:project) { create(:project) }
+  let(:project) { create(:project) }
 
   let(:response) do
     Gitlab::PhabricatorImport::Conduit::TasksResponse.new(
@@ -15,7 +15,6 @@ describe Gitlab::PhabricatorImport::Issues::Importer do
 
   before do
     client = instance_double(Gitlab::PhabricatorImport::Conduit::Maniphest)
-
     allow(client).to receive(:tasks).and_return(response)
     allow(importer).to receive(:client).and_return(client)
   end
@@ -34,20 +33,29 @@ describe Gitlab::PhabricatorImport::Issues::Importer do
       importer.execute
     end
 
-    it 'schedules the next batch if there is one' do
-      expect(Gitlab::PhabricatorImport::ImportTasksWorker)
-        .to receive(:schedule).with(project.id, response.pagination.next_page)
+    context 'stubbed task import' do
+      before do
+        # Stub out the actual importing so we don't perform aditional requests
+        expect_next_instance_of(Gitlab::PhabricatorImport::Issues::TaskImporter) do |task_importer|
+          allow(task_importer).to receive(:execute)
+        end.at_least(1)
+      end
 
-      importer.execute
-    end
+      it 'schedules the next batch if there is one' do
+        expect(Gitlab::PhabricatorImport::ImportTasksWorker)
+          .to receive(:schedule).with(project.id, response.pagination.next_page)
 
-    it 'does not reschedule when there is no next page' do
-      allow(response.pagination).to receive(:has_next_page?).and_return(false)
+        importer.execute
+      end
 
-      expect(Gitlab::PhabricatorImport::ImportTasksWorker)
-        .not_to receive(:schedule)
+      it 'does not reschedule when there is no next page' do
+        allow(response.pagination).to receive(:has_next_page?).and_return(false)
 
-      importer.execute
+        expect(Gitlab::PhabricatorImport::ImportTasksWorker)
+          .not_to receive(:schedule)
+
+        importer.execute
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/phabricator_import/issues/task_importer_spec.rb b/spec/lib/gitlab/phabricator_import/issues/task_importer_spec.rb
index 1625604e754..06ed264e781 100644
--- a/spec/lib/gitlab/phabricator_import/issues/task_importer_spec.rb
+++ b/spec/lib/gitlab/phabricator_import/issues/task_importer_spec.rb
@@ -12,6 +12,8 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do
           'description' => {
             'raw' => '# This is markdown\n it can contain more text.'
           },
+          'authorPHID' => 'PHID-USER-456',
+          'ownerPHID' => 'PHID-USER-123',
           'dateCreated' => '1518688921',
           'dateClosed' => '1518789995'
         }
@@ -19,9 +21,18 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do
     )
   end
 
+  subject(:importer) { described_class.new(project, task) }
+
   describe '#execute' do
+    let(:fake_user_finder) { instance_double(Gitlab::PhabricatorImport::UserFinder) }
+
+    before do
+      allow(fake_user_finder).to receive(:find)
+      allow(importer).to receive(:user_finder).and_return(fake_user_finder)
+    end
+
     it 'creates the issue with the expected attributes' do
-      issue = described_class.new(project, task).execute
+      issue = importer.execute
 
       expect(issue.project).to eq(project)
       expect(issue).to be_persisted
@@ -34,21 +45,38 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do
     end
 
     it 'does not recreate the issue when called multiple times' do
-      expect { described_class.new(project, task).execute }
+      expect { importer.execute }
         .to change { project.issues.reload.size }.from(0).to(1)
-      expect { described_class.new(project, task).execute }
+      expect { importer.execute }
         .not_to change { project.issues.reload.size }
     end
 
     it 'does not trigger a save when the object did not change' do
       existing_issue = create(:issue,
                               task.issue_attributes.merge(author: User.ghost))
-      importer = described_class.new(project, task)
       allow(importer).to receive(:issue).and_return(existing_issue)
 
       expect(existing_issue).not_to receive(:save!)
 
       importer.execute
     end
+
+    it 'links the author if the author can be found' do
+      author = create(:user)
+      expect(fake_user_finder).to receive(:find).with('PHID-USER-456').and_return(author)
+
+      issue = importer.execute
+
+      expect(issue.author).to eq(author)
+    end
+
+    it 'links an assignee if the user can be found' do
+      assignee = create(:user)
+      expect(fake_user_finder).to receive(:find).with('PHID-USER-123').and_return(assignee)
+
+      issue = importer.execute
+
+      expect(issue.assignees).to include(assignee)
+    end
   end
 end
diff --git a/spec/lib/gitlab/phabricator_import/representation/task_spec.rb b/spec/lib/gitlab/phabricator_import/representation/task_spec.rb
index dfbd8c546eb..5603a6961d6 100644
--- a/spec/lib/gitlab/phabricator_import/representation/task_spec.rb
+++ b/spec/lib/gitlab/phabricator_import/representation/task_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 describe Gitlab::PhabricatorImport::Representation::Task do
@@ -7,6 +9,8 @@ describe Gitlab::PhabricatorImport::Representation::Task do
         'phid' => 'the-phid',
         'fields' => {
           'name' => 'Title'.ljust(257, '.'), # A string padded to 257 chars
+          'authorPHID' => 'a phid',
+          'ownerPHID' => 'another user phid',
           'description' => {
             'raw' => '# This is markdown\n it can contain more text.'
           },
@@ -30,4 +34,16 @@ describe Gitlab::PhabricatorImport::Representation::Task do
       expect(task.issue_attributes).to eq(expected_attributes)
     end
   end
+
+  describe '#author_phid' do
+    it 'returns the correct field' do
+      expect(task.author_phid).to eq('a phid')
+    end
+  end
+
+  describe '#owner_phid' do
+    it 'returns the correct field' do
+      expect(task.owner_phid).to eq('another user phid')
+    end
+  end
 end
diff --git a/spec/lib/gitlab/phabricator_import/representation/user_spec.rb b/spec/lib/gitlab/phabricator_import/representation/user_spec.rb
new file mode 100644
index 00000000000..f52467a0cf1
--- /dev/null
+++ b/spec/lib/gitlab/phabricator_import/representation/user_spec.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::PhabricatorImport::Representation::User do
+  subject(:user) do
+    described_class.new(
+      {
+        'phid' => 'the-phid',
+        'fields' => {
+          'username' => 'the-username'
+        }
+      }
+    )
+  end
+
+  describe '#phabricator_id' do
+    it 'returns the phabricator id' do
+      expect(user.phabricator_id).to eq('the-phid')
+    end
+  end
+
+  describe '#username' do
+    it 'returns the username' do
+      expect(user.username).to eq('the-username')
+    end
+  end
+end
diff --git a/spec/lib/gitlab/phabricator_import/user_finder_spec.rb b/spec/lib/gitlab/phabricator_import/user_finder_spec.rb
new file mode 100644
index 00000000000..096321cda5f
--- /dev/null
+++ b/spec/lib/gitlab/phabricator_import/user_finder_spec.rb
@@ -0,0 +1,89 @@
+require 'spec_helper'
+
+describe Gitlab::PhabricatorImport::UserFinder, :clean_gitlab_redis_cache do
+  let(:project) { create(:project, namespace: create(:group)) }
+  subject(:finder) { described_class.new(project, ['first-phid', 'second-phid']) }
+
+  before do
+    project.namespace.add_developer(existing_user)
+  end
+
+  describe '#find' do
+    let!(:existing_user) { create(:user, username: 'existing-user') }
+    let(:cache) { Gitlab::PhabricatorImport::Cache::Map.new(project) }
+
+    before do
+      allow(finder).to receive(:object_map).and_return(cache)
+    end
+
+    context 'for a cached phid' do
+      before do
+        cache.set_gitlab_model(existing_user, 'first-phid')
+      end
+
+      it 'returns the existing user' do
+        expect(finder.find('first-phid')).to eq(existing_user)
+      end
+
+      it 'does not perform a find using the API' do
+        expect(finder).not_to receive(:find_user_for_phid)
+
+        finder.find('first-phid')
+      end
+
+      it 'excludes the phid from the request if one needs to be made' do
+        client = instance_double(Gitlab::PhabricatorImport::Conduit::User)
+        allow(finder).to receive(:client).and_return(client)
+
+        expect(client).to receive(:users).with(['second-phid']).and_return([])
+
+        finder.find('first-phid')
+        finder.find('second-phid')
+      end
+    end
+
+    context 'when the phid is not cached' do
+      let(:response) do
+        [
+          instance_double(
+            Gitlab::PhabricatorImport::Conduit::UsersResponse,
+            users: [instance_double(Gitlab::PhabricatorImport::Representation::User, phabricator_id: 'second-phid', username: 'existing-user')]
+          ),
+          instance_double(
+            Gitlab::PhabricatorImport::Conduit::UsersResponse,
+            users: [instance_double(Gitlab::PhabricatorImport::Representation::User, phabricator_id: 'first-phid', username: 'other-user')]
+          )
+        ]
+      end
+      let(:client) do
+        client = instance_double(Gitlab::PhabricatorImport::Conduit::User)
+        allow(client).to receive(:users).and_return(response)
+
+        client
+      end
+
+      before do
+        allow(finder).to receive(:client).and_return(client)
+      end
+
+      it 'loads the users from the API once' do
+        expect(client).to receive(:users).and_return(response).once
+
+        expect(finder.find('second-phid')).to eq(existing_user)
+        expect(finder.find('first-phid')).to be_nil
+      end
+
+      it 'adds found users to the cache' do
+        expect { finder.find('second-phid') }
+          .to change { cache.get_gitlab_model('second-phid') }
+                .from(nil).to(existing_user)
+      end
+
+      it 'only returns users that are members of the project' do
+        create(:user, username: 'other-user')
+
+        expect(finder.find('first-phid')).to eq(nil)
+      end
+    end
+  end
+end