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

doc/user/project/import/phabricator.md

@@ -15,6 +15,12 @@ Currently, only the following basic fields are imported:
 - Created at
 - Closed at
 
+## Users
+
+The assignee and author of a user are deducted from a Task's owner and
+author: If a user with the same username has access to the namespace
+of the project being imported into, then the user will be linked.
+
 ## Enabling this feature
 
 While this feature is incomplete, a feature flag is required to enable it so that

lib/gitlab/phabricator_import/cache/map.rb

@@ -9,9 +9,15 @@ module Gitlab
 
         def get_gitlab_model(phabricator_id)
           cached_info = get(phabricator_id)
-          return unless cached_info[:classname] && cached_info[:database_id]
 
-          cached_info[:classname].constantize.find_by_id(cached_info[:database_id])
+          if cached_info[:classname] && cached_info[:database_id]
+            object = cached_info[:classname].constantize.find_by_id(cached_info[:database_id])
+          else
+            object = yield if block_given?
+            set_gitlab_model(object, phabricator_id) if object
+          end
+
+          object
         end
 
         def set_gitlab_model(object, phabricator_id)

lib/gitlab/phabricator_import/conduit/user.rb

+# frozen_string_literal: true
+module Gitlab
+  module PhabricatorImport
+    module Conduit
+      class User
+        MAX_PAGE_SIZE = 100
+
+        def initialize(phabricator_url:, api_token:)
+          @client = Client.new(phabricator_url, api_token)
+        end
+
+        def users(phids)
+          phids.each_slice(MAX_PAGE_SIZE).map { |limited_phids| get_page(limited_phids) }
+        end
+
+        private
+
+        def get_page(phids)
+          UsersResponse.new(get_users(phids))
+        end
+
+        def get_users(phids)
+          client.get('user.search',
+                     params: { constraints: { phids: phids } })
+        end
+
+        attr_reader :client
+      end
+    end
+  end
+end

lib/gitlab/phabricator_import/conduit/users_response.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module PhabricatorImport
+    module Conduit
+      class UsersResponse
+        def initialize(conduit_response)
+          @conduit_response = conduit_response
+        end
+
+        def users
+          @users ||= conduit_response.data.map do |user_json|
+            Gitlab::PhabricatorImport::Representation::User.new(user_json)
+          end
+        end
+
+        private
+
+        attr_reader :conduit_response
+      end
+    end
+  end
+end

lib/gitlab/phabricator_import/issues/task_importer.rb

@@ -8,9 +8,7 @@ module Gitlab
         end
 
         def execute
-          # TODO: get the user from the project namespace from the username loaded by Phab-id
-          # https://gitlab.com/gitlab-org/gitlab-ce/issues/60565
-          issue.author = User.ghost
+          issue.author = user_finder.find(task.author_phid) || User.ghost
 
           # TODO: Reformat the description with attachments, escaping accidental
           # links and add attachments
@@ -19,6 +17,10 @@ module Gitlab
 
           save!
 
+          if owner = user_finder.find(task.owner_phid)
+            issue.assignees << owner
+          end
+
           issue
         end
 
@@ -41,6 +43,10 @@ module Gitlab
             project.issues.new
         end
 
+        def user_finder
+          @issue_finder ||= Gitlab::PhabricatorImport::UserFinder.new(project, task.phids)
+        end
+
         def find_issue_by_phabricator_id(phabricator_id)
           object_map.get_gitlab_model(phabricator_id)
         end

lib/gitlab/phabricator_import/representation/task.rb

@@ -11,6 +11,18 @@ module Gitlab
           json['phid']
         end
 
+        def author_phid
+          json['fields']['authorPHID']
+        end
+
+        def owner_phid
+          json['fields']['ownerPHID']
+        end
+
+        def phids
+          @phids ||= [author_phid, owner_phid]
+        end
+
         def issue_attributes
           @issue_attributes ||= {
             title: issue_title,

lib/gitlab/phabricator_import/representation/user.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module PhabricatorImport
+    module Representation
+      class User
+        def initialize(json)
+          @json = json
+        end
+
+        def phabricator_id
+          json['phid']
+        end
+
+        def username
+          json['fields']['username']
+        end
+
+        private
+
+        attr_reader :json
+      end
+    end
+  end
+end

lib/gitlab/phabricator_import/user_finder.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module PhabricatorImport
+    class UserFinder
+      def initialize(project, phids)
+        @project, @phids = project, phids
+        @loaded_phids = Set.new
+      end
+
+      def find(phid)
+        found_user = object_map.get_gitlab_model(phid) do
+          find_user_for_phid(phid)
+        end
+
+        loaded_phids << phid
+
+        found_user
+      end
+
+      private
+
+      attr_reader :project, :phids, :loaded_phids
+
+      def object_map
+        @object_map ||= Gitlab::PhabricatorImport::Cache::Map.new(project)
+      end
+
+      def find_user_for_phid(phid)
+        phabricator_user = phabricator_users.find { |u| u.phabricator_id == phid }
+        return unless phabricator_user
+
+        project.authorized_users.find_by_username(phabricator_user.username)
+      end
+
+      def phabricator_users
+        @user_responses ||= client.users(users_to_request).flat_map(&:users)
+      end
+
+      def users_to_request
+        phids - loaded_phids.to_a
+      end
+
+      def client
+        @client ||=
+          Gitlab::PhabricatorImport::Conduit::User
+            .new(phabricator_url: project.import_data.data['phabricator_url'],
+                 api_token: project.import_data.credentials[:api_token])
+      end
+    end
+  end
+end

spec/fixtures/phabricator_responses/user.search.json

+{
+  "result": {
+    "data": [
+      {
+        "id": 1,
+        "type": "USER",
+        "phid": "PHID-USER-hohoho",
+        "fields": {
+          "username": "jane",
+          "realName": "Jane Doe",
+          "roles": [
+            "admin",
+            "verified",
+            "approved",
+            "activated"
+          ],
+          "dateCreated": 1405970599,
+          "dateModified": 1406705963,
+          "policy": {
+            "view": "public",
+            "edit": "no-one"
+          }
+        },
+        "attachments": {}
+      },
+      {
+        "id": 2,
+        "type": "USER",
+        "phid": "PHID-USER-hihihi",
+        "fields": {
+          "username": "john",
+          "realName": "John Doe",
+          "roles": [
+            "admin",
+            "verified",
+            "approved",
+            "activated"
+          ],
+          "dateCreated": 1403609184,
+          "dateModified": 1559138722,
+          "policy": {
+            "view": "public",
+            "edit": "no-one"
+          }
+        },
+        "attachments": {}
+      }
+    ],
+    "maps": {},
+    "query": {
+      "queryKey": null
+    },
+    "cursor": {
+      "limit": "100",
+      "after": null,
+      "before": null,
+      "order": null
+    }
+  },
+  "error_code": null,
+  "error_info": null
+}

spec/lib/gitlab/phabricator_import/cache/map_spec.rb

+# 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

spec/lib/gitlab/phabricator_import/conduit/user_spec.rb

+# 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

spec/lib/gitlab/phabricator_import/conduit/users_response_spec.rb

+# 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

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

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

spec/lib/gitlab/phabricator_import/representation/task_spec.rb

+# 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

spec/lib/gitlab/phabricator_import/representation/user_spec.rb

+# 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

spec/lib/gitlab/phabricator_import/user_finder_spec.rb

+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