From 32184839c3983babe53ea93fc16b7cde5ada95f6 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 3 Jul 2019 19:44:05 +0200 Subject: [PATCH] Fetch users from Phabricator to link to issues We fetch the users from Phabricator based on their Phabricator ID. If a user with the same username exists and is a member of the project, we set them as assignee or author. When a user is applicable, we also cache it in Redis so we don't have to perform the request again for the same phid. --- doc/user/project/import/phabricator.md | 6 ++ lib/gitlab/phabricator_import/cache/map.rb | 10 ++- lib/gitlab/phabricator_import/conduit/user.rb | 31 +++++++ .../conduit/users_response.rb | 23 +++++ .../issues/task_importer.rb | 12 ++- .../phabricator_import/representation/task.rb | 12 +++ .../phabricator_import/representation/user.rb | 25 ++++++ lib/gitlab/phabricator_import/user_finder.rb | 52 +++++++++++ .../phabricator_responses/user.search.json | 62 +++++++++++++ .../phabricator_import/cache/map_spec.rb | 17 ++++ .../phabricator_import/conduit/user_spec.rb | 49 ++++++++++ .../conduit/users_response_spec.rb | 21 +++++ .../issues/importer_spec.rb | 32 ++++--- .../issues/task_importer_spec.rb | 36 +++++++- .../representation/task_spec.rb | 16 ++++ .../representation/user_spec.rb | 28 ++++++ .../phabricator_import/user_finder_spec.rb | 89 +++++++++++++++++++ 17 files changed, 500 insertions(+), 21 deletions(-) create mode 100644 lib/gitlab/phabricator_import/conduit/user.rb create mode 100644 lib/gitlab/phabricator_import/conduit/users_response.rb create mode 100644 lib/gitlab/phabricator_import/representation/user.rb create mode 100644 lib/gitlab/phabricator_import/user_finder.rb create mode 100644 spec/fixtures/phabricator_responses/user.search.json create mode 100644 spec/lib/gitlab/phabricator_import/conduit/user_spec.rb create mode 100644 spec/lib/gitlab/phabricator_import/conduit/users_response_spec.rb create mode 100644 spec/lib/gitlab/phabricator_import/representation/user_spec.rb create mode 100644 spec/lib/gitlab/phabricator_import/user_finder_spec.rb diff --git a/doc/user/project/import/phabricator.md b/doc/user/project/import/phabricator.md index 5c624e3aff6..b8f89caba24 100644 --- a/doc/user/project/import/phabricator.md +++ b/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 diff --git a/lib/gitlab/phabricator_import/cache/map.rb b/lib/gitlab/phabricator_import/cache/map.rb index fa8b37b20ca..6a2841b6a8e 100644 --- a/lib/gitlab/phabricator_import/cache/map.rb +++ b/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) diff --git a/lib/gitlab/phabricator_import/conduit/user.rb b/lib/gitlab/phabricator_import/conduit/user.rb new file mode 100644 index 00000000000..fc8c3f7cde9 --- /dev/null +++ b/lib/gitlab/phabricator_import/conduit/user.rb @@ -0,0 +1,31 @@ +# 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 diff --git a/lib/gitlab/phabricator_import/conduit/users_response.rb b/lib/gitlab/phabricator_import/conduit/users_response.rb new file mode 100644 index 00000000000..3dfb29a7be5 --- /dev/null +++ b/lib/gitlab/phabricator_import/conduit/users_response.rb @@ -0,0 +1,23 @@ +# 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 diff --git a/lib/gitlab/phabricator_import/issues/task_importer.rb b/lib/gitlab/phabricator_import/issues/task_importer.rb index 40d4392cbc1..77ee11c7cdd 100644 --- a/lib/gitlab/phabricator_import/issues/task_importer.rb +++ b/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 diff --git a/lib/gitlab/phabricator_import/representation/task.rb b/lib/gitlab/phabricator_import/representation/task.rb index 6aedc71b626..ba93fb37a8e 100644 --- a/lib/gitlab/phabricator_import/representation/task.rb +++ b/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, diff --git a/lib/gitlab/phabricator_import/representation/user.rb b/lib/gitlab/phabricator_import/representation/user.rb new file mode 100644 index 00000000000..7fd7cecc6ae --- /dev/null +++ b/lib/gitlab/phabricator_import/representation/user.rb @@ -0,0 +1,25 @@ +# 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 diff --git a/lib/gitlab/phabricator_import/user_finder.rb b/lib/gitlab/phabricator_import/user_finder.rb new file mode 100644 index 00000000000..4b50431e0e0 --- /dev/null +++ b/lib/gitlab/phabricator_import/user_finder.rb @@ -0,0 +1,52 @@ +# 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 diff --git a/spec/fixtures/phabricator_responses/user.search.json b/spec/fixtures/phabricator_responses/user.search.json new file mode 100644 index 00000000000..f3ec653a23e --- /dev/null +++ b/spec/fixtures/phabricator_responses/user.search.json @@ -0,0 +1,62 @@ +{ + "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 +} 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