From 32184839c3983babe53ea93fc16b7cde5ada95f6 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 3 Jul 2019 19:44:05 +0200 Subject: 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. --- .../gitlab/phabricator_import/cache/map_spec.rb | 17 +++++ .../gitlab/phabricator_import/conduit/user_spec.rb | 49 ++++++++++++ .../conduit/users_response_spec.rb | 21 +++++ .../phabricator_import/issues/importer_spec.rb | 32 +++++--- .../issues/task_importer_spec.rb | 36 ++++++++- .../phabricator_import/representation/task_spec.rb | 16 ++++ .../phabricator_import/representation/user_spec.rb | 28 +++++++ .../gitlab/phabricator_import/user_finder_spec.rb | 89 ++++++++++++++++++++++ 8 files changed, 272 insertions(+), 16 deletions(-) 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 (limited to 'spec/lib/gitlab/phabricator_import') 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 -- cgit v1.2.3