Commit 32184839 authored by Bob Van Landuyt's avatar Bob Van Landuyt

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.
parent e787676b
...@@ -15,6 +15,12 @@ Currently, only the following basic fields are imported: ...@@ -15,6 +15,12 @@ Currently, only the following basic fields are imported:
- Created at - Created at
- Closed 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 ## Enabling this feature
While this feature is incomplete, a feature flag is required to enable it so that While this feature is incomplete, a feature flag is required to enable it so that
......
...@@ -9,9 +9,15 @@ module Gitlab ...@@ -9,9 +9,15 @@ module Gitlab
def get_gitlab_model(phabricator_id) def get_gitlab_model(phabricator_id)
cached_info = get(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 end
def set_gitlab_model(object, phabricator_id) def set_gitlab_model(object, phabricator_id)
......
# 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
# 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
...@@ -8,9 +8,7 @@ module Gitlab ...@@ -8,9 +8,7 @@ module Gitlab
end end
def execute def execute
# TODO: get the user from the project namespace from the username loaded by Phab-id issue.author = user_finder.find(task.author_phid) || User.ghost
# https://gitlab.com/gitlab-org/gitlab-ce/issues/60565
issue.author = User.ghost
# TODO: Reformat the description with attachments, escaping accidental # TODO: Reformat the description with attachments, escaping accidental
# links and add attachments # links and add attachments
...@@ -19,6 +17,10 @@ module Gitlab ...@@ -19,6 +17,10 @@ module Gitlab
save! save!
if owner = user_finder.find(task.owner_phid)
issue.assignees << owner
end
issue issue
end end
...@@ -41,6 +43,10 @@ module Gitlab ...@@ -41,6 +43,10 @@ module Gitlab
project.issues.new project.issues.new
end end
def user_finder
@issue_finder ||= Gitlab::PhabricatorImport::UserFinder.new(project, task.phids)
end
def find_issue_by_phabricator_id(phabricator_id) def find_issue_by_phabricator_id(phabricator_id)
object_map.get_gitlab_model(phabricator_id) object_map.get_gitlab_model(phabricator_id)
end end
......
...@@ -11,6 +11,18 @@ module Gitlab ...@@ -11,6 +11,18 @@ module Gitlab
json['phid'] json['phid']
end 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 def issue_attributes
@issue_attributes ||= { @issue_attributes ||= {
title: issue_title, title: issue_title,
......
# 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
# 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
{
"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
}
# frozen_string_literal: true
require 'spec_helper' require 'spec_helper'
describe Gitlab::PhabricatorImport::Cache::Map, :clean_gitlab_redis_cache do describe Gitlab::PhabricatorImport::Cache::Map, :clean_gitlab_redis_cache do
...@@ -28,6 +30,21 @@ 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 expect(ttl).to be > 10.seconds
end 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 end
describe '#set_gitlab_model' do describe '#set_gitlab_model' do
......
# 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
# 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
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
describe Gitlab::PhabricatorImport::Issues::Importer do describe Gitlab::PhabricatorImport::Issues::Importer do
set(:project) { create(:project) } let(:project) { create(:project) }
let(:response) do let(:response) do
Gitlab::PhabricatorImport::Conduit::TasksResponse.new( Gitlab::PhabricatorImport::Conduit::TasksResponse.new(
...@@ -15,7 +15,6 @@ describe Gitlab::PhabricatorImport::Issues::Importer do ...@@ -15,7 +15,6 @@ describe Gitlab::PhabricatorImport::Issues::Importer do
before do before do
client = instance_double(Gitlab::PhabricatorImport::Conduit::Maniphest) client = instance_double(Gitlab::PhabricatorImport::Conduit::Maniphest)
allow(client).to receive(:tasks).and_return(response) allow(client).to receive(:tasks).and_return(response)
allow(importer).to receive(:client).and_return(client) allow(importer).to receive(:client).and_return(client)
end end
...@@ -34,6 +33,14 @@ describe Gitlab::PhabricatorImport::Issues::Importer do ...@@ -34,6 +33,14 @@ describe Gitlab::PhabricatorImport::Issues::Importer do
importer.execute importer.execute
end end
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
it 'schedules the next batch if there is one' do it 'schedules the next batch if there is one' do
expect(Gitlab::PhabricatorImport::ImportTasksWorker) expect(Gitlab::PhabricatorImport::ImportTasksWorker)
.to receive(:schedule).with(project.id, response.pagination.next_page) .to receive(:schedule).with(project.id, response.pagination.next_page)
...@@ -50,4 +57,5 @@ describe Gitlab::PhabricatorImport::Issues::Importer do ...@@ -50,4 +57,5 @@ describe Gitlab::PhabricatorImport::Issues::Importer do
importer.execute importer.execute
end end
end end
end
end end
...@@ -12,6 +12,8 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do ...@@ -12,6 +12,8 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do
'description' => { 'description' => {
'raw' => '# This is markdown\n it can contain more text.' 'raw' => '# This is markdown\n it can contain more text.'
}, },
'authorPHID' => 'PHID-USER-456',
'ownerPHID' => 'PHID-USER-123',
'dateCreated' => '1518688921', 'dateCreated' => '1518688921',
'dateClosed' => '1518789995' 'dateClosed' => '1518789995'
} }
...@@ -19,9 +21,18 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do ...@@ -19,9 +21,18 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do
) )
end end
subject(:importer) { described_class.new(project, task) }
describe '#execute' do 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 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.project).to eq(project)
expect(issue).to be_persisted expect(issue).to be_persisted
...@@ -34,21 +45,38 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do ...@@ -34,21 +45,38 @@ describe Gitlab::PhabricatorImport::Issues::TaskImporter do
end end
it 'does not recreate the issue when called multiple times' do 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) .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 } .not_to change { project.issues.reload.size }
end end
it 'does not trigger a save when the object did not change' do it 'does not trigger a save when the object did not change' do
existing_issue = create(:issue, existing_issue = create(:issue,
task.issue_attributes.merge(author: User.ghost)) task.issue_attributes.merge(author: User.ghost))
importer = described_class.new(project, task)
allow(importer).to receive(:issue).and_return(existing_issue) allow(importer).to receive(:issue).and_return(existing_issue)
expect(existing_issue).not_to receive(:save!) expect(existing_issue).not_to receive(:save!)
importer.execute importer.execute
end 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
end end
# frozen_string_literal: true
require 'spec_helper' require 'spec_helper'
describe Gitlab::PhabricatorImport::Representation::Task do describe Gitlab::PhabricatorImport::Representation::Task do
...@@ -7,6 +9,8 @@ describe Gitlab::PhabricatorImport::Representation::Task do ...@@ -7,6 +9,8 @@ describe Gitlab::PhabricatorImport::Representation::Task do
'phid' => 'the-phid', 'phid' => 'the-phid',
'fields' => { 'fields' => {
'name' => 'Title'.ljust(257, '.'), # A string padded to 257 chars 'name' => 'Title'.ljust(257, '.'), # A string padded to 257 chars
'authorPHID' => 'a phid',
'ownerPHID' => 'another user phid',
'description' => { 'description' => {
'raw' => '# This is markdown\n it can contain more text.' 'raw' => '# This is markdown\n it can contain more text.'
}, },
...@@ -30,4 +34,16 @@ describe Gitlab::PhabricatorImport::Representation::Task do ...@@ -30,4 +34,16 @@ describe Gitlab::PhabricatorImport::Representation::Task do
expect(task.issue_attributes).to eq(expected_attributes) expect(task.issue_attributes).to eq(expected_attributes)
end end
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 end
# 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
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment