Refactoring GithubImport::Importer

lib/gitlab/github_import/comment.rb

+module Gitlab
+  module GithubImport
+    class Comment
+      attr_reader :project, :raw_data
+
+      def initialize(project, raw_data)
+        @project = project
+        @raw_data = raw_data
+        @formatter = Gitlab::ImportFormatter.new
+      end
+
+      def attributes
+        {
+          project: project,
+          note: note,
+          commit_id: raw_data.commit_id,
+          line_code: line_code,
+          author_id: author_id,
+          created_at: raw_data.created_at,
+          updated_at: raw_data.updated_at
+        }
+      end
+
+      private
+
+      def author
+        raw_data.user.login
+      end
+
+      def author_id
+        gl_user_id(raw_data.user.id) || project.creator_id
+      end
+
+      def body
+        raw_data.body || ""
+      end
+
+      def line_code
+        if on_diff?
+          Gitlab::Diff::LineCode.generate(raw_data.path, raw_data.position, 0)
+        end
+      end
+
+      def on_diff?
+        raw_data.path && raw_data.position
+      end
+
+      def note
+        @formatter.author_line(author) + body
+      end
+
+      def gl_user_id(github_id)
+        User.joins(:identities).
+          find_by("identities.extern_uid = ? AND identities.provider = 'github'", github_id.to_s).
+          try(:id)
+      end
+    end
+  end
+end

lib/gitlab/github_import/importer.rb

@@ -51,80 +51,31 @@ module Gitlab
       def import_pull_requests
         client.pull_requests(project.import_source, state: :all,
                                                     sort: :created,
-                                                    direction: :asc).each do |pull_request|
-          source_branch = find_branch(pull_request.head.ref)
-          target_branch = find_branch(pull_request.base.ref)
+                                                    direction: :asc).each do |raw_data|
+          pull_request = PullRequest.new(project, raw_data)
 
-          if source_branch && target_branch
-            merge_request = MergeRequest.create!(
-              title: pull_request.title,
-              description: format_body(pull_request.user.login, pull_request.body),
-              source_project: project,
-              source_branch: source_branch.name,
-              target_project: project,
-              target_branch: target_branch.name,
-              state: merge_request_state(pull_request),
-              author_id: gl_author_id(project, pull_request.user.id),
-              assignee_id: gl_user_id(pull_request.assignee.try(:id)),
-              created_at: pull_request.created_at,
-              updated_at: pull_request.updated_at
-            )
-
-            import_comments_on_pull_request(merge_request, pull_request)
-            import_comments_on_pull_request_diff(merge_request, pull_request)
+          if pull_request.valid?
+            merge_request = MergeRequest.create!(pull_request.attributes)
+            import_comments_on_pull_request(merge_request, raw_data)
+            import_comments_on_pull_request_diff(merge_request, raw_data)
           end
         end
       end
 
       def import_comments_on_pull_request(merge_request, pull_request)
-        client.issue_comments(project.import_source, pull_request.number).each do |c|
-          merge_request.notes.create!(
-            project: project,
-            note: format_body(c.user.login, c.body),
-            author_id: gl_author_id(project, c.user.id),
-            created_at: c.created_at,
-            updated_at: c.updated_at
-          )
+        client.issue_comments(project.import_source, pull_request.number).each do |raw_data|
+          comment = Comment.new(project, raw_data)
+          merge_request.notes.create!(comment.attributes)
         end
       end
 
       def import_comments_on_pull_request_diff(merge_request, pull_request)
-        client.pull_request_comments(project.import_source, pull_request.number).each do |c|
-          merge_request.notes.create!(
-            project: project,
-            note: format_body(c.user.login, c.body),
-            commit_id: c.commit_id,
-            line_code: generate_line_code(c.path, c.position),
-            author_id: gl_author_id(project, c.user.id),
-            created_at: c.created_at,
-            updated_at: c.updated_at
-          )
+        client.pull_request_comments(project.import_source, pull_request.number).each do |raw_data|
+          comment = Comment.new(project, raw_data)
+          merge_request.notes.create!(comment.attributes)
         end
       end
 
-      def find_branch(name)
-        project.repository.find_branch(name)
-      end
-
-      def format_body(author, body)
-        @formatter.author_line(author) + (body || "")
-      end
-
-      def merge_request_state(pull_request)
-        case true
-        when pull_request.state == 'closed' && pull_request.merged_at.present?
-          'merged'
-        when pull_request.state == 'closed'
-          'closed'
-        else
-          'opened'
-        end
-      end
-
-      def generate_line_code(file_path, position)
-        Gitlab::Diff::LineCode.generate(file_path, position, 0)
-      end
-
       def gl_author_id(project, github_id)
         gl_user_id(github_id) || project.creator_id
       end

lib/gitlab/github_import/pull_request.rb

+module Gitlab
+  module GithubImport
+    class PullRequest
+      attr_reader :project, :raw_data
+
+      def initialize(project, raw_data)
+        @project = project
+        @raw_data = raw_data
+        @formatter = Gitlab::ImportFormatter.new
+      end
+
+      def attributes
+        {
+          title: raw_data.title,
+          description: description,
+          source_project: source_project,
+          source_branch: source_branch.name,
+          target_project: target_project,
+          target_branch: target_branch.name,
+          state: state,
+          author_id: author_id,
+          assignee_id: assignee_id,
+          created_at: raw_data.created_at,
+          updated_at: updated_at
+        }
+      end
+
+      def valid?
+        source_branch.present? && target_branch.present?
+      end
+
+      private
+
+      def assigned?
+        raw_data.assignee.present?
+      end
+
+      def assignee_id
+        if assigned?
+          gl_user_id(raw_data.assignee.id)
+        end
+      end
+
+      def author
+        raw_data.user.login
+      end
+
+      def author_id
+        gl_user_id(raw_data.user.id) || project.creator_id
+      end
+
+      def body
+        raw_data.body || ""
+      end
+
+      def description
+        @formatter.author_line(author) + body
+      end
+
+      def source_project
+        project
+      end
+
+      def source_branch
+        source_project.repository.find_branch(raw_data.head.ref)
+      end
+
+      def target_project
+        project
+      end
+
+      def target_branch
+        target_project.repository.find_branch(raw_data.base.ref)
+      end
+
+      def state
+        @state ||= case true
+                   when raw_data.state == 'closed' && raw_data.merged_at.present?
+                     'merged'
+                   when raw_data.state == 'closed'
+                     'closed'
+                   else
+                     'opened'
+                   end
+      end
+
+      def updated_at
+        case state
+        when 'merged' then raw_data.merged_at
+        when 'closed' then raw_data.closed_at
+        else
+          raw_data.updated_at
+        end
+      end
+
+      def gl_user_id(github_id)
+        User.joins(:identities).
+          find_by("identities.extern_uid = ? AND identities.provider = 'github'", github_id.to_s).
+          try(:id)
+      end
+    end
+  end
+end

spec/lib/gitlab/github_import/comment_spec.rb

+require 'spec_helper'
+
+describe Gitlab::GithubImport::Comment, lib: true do
+  let(:project) { create(:project) }
+  let(:octocat) { OpenStruct.new(id: 123456, login: 'octocat') }
+  let(:created_at) { DateTime.strptime('2013-04-10T20:09:31Z') }
+  let(:updated_at) { DateTime.strptime('2014-03-03T18:58:10Z') }
+  let(:base_data) do
+    {
+      body: "I'm having a problem with this.",
+      user: octocat,
+      created_at: created_at,
+      updated_at: updated_at
+    }
+  end
+
+  subject(:comment) { described_class.new(project, raw_data)}
+
+  describe '#attributes' do
+    context 'when do not reference a portion of the diff' do
+      let(:raw_data) { OpenStruct.new(base_data) }
+
+      it 'returns formatted attributes' do
+        expected = {
+          project: project,
+          note: "*Created by: octocat*\n\nI'm having a problem with this.",
+          commit_id: nil,
+          line_code: nil,
+          author_id: project.creator_id,
+          created_at: created_at,
+          updated_at: updated_at
+        }
+
+        expect(comment.attributes).to eq(expected)
+      end
+    end
+
+    context 'when on a portion of the diff' do
+      let(:diff_data) do
+        {
+          body: 'Great stuff',
+          commit_id: '6dcb09b5b57875f334f61aebed695e2e4193db5e',
+          diff_hunk: '@@ -16,33 +16,40 @@ public class Connection : IConnection...',
+          path: 'file1.txt',
+          position: 1
+        }
+      end
+
+      let(:raw_data) { OpenStruct.new(base_data.merge(diff_data)) }
+
+      it 'returns formatted attributes' do
+        expected = {
+          project: project,
+          note: "*Created by: octocat*\n\nGreat stuff",
+          commit_id: '6dcb09b5b57875f334f61aebed695e2e4193db5e',
+          line_code: 'ce1be0ff4065a6e9415095c95f25f47a633cef2b_0_1',
+          author_id: project.creator_id,
+          created_at: created_at,
+          updated_at: updated_at
+        }
+
+        expect(comment.attributes).to eq(expected)
+      end
+    end
+
+    context 'when author is a GitLab user' do
+      let(:raw_data) { OpenStruct.new(base_data.merge(user: octocat)) }
+
+      it 'returns project#creator_id as author_id when is not a GitLab user' do
+        expect(comment.attributes.fetch(:author_id)).to eq project.creator_id
+      end
+
+      it 'returns GitLab user id as author_id when is a GitLab user' do
+        gl_user = create(:omniauth_user, extern_uid: octocat.id, provider: 'github')
+
+        expect(comment.attributes.fetch(:author_id)).to eq gl_user.id
+      end
+    end
+  end
+end

spec/lib/gitlab/github_import/pull_request_spec.rb

+require 'spec_helper'
+
+describe Gitlab::GithubImport::PullRequest, lib: true do
+  let(:project) { create(:project) }
+  let(:source_branch) { OpenStruct.new(ref: 'feature') }
+  let(:target_branch) { OpenStruct.new(ref: 'master') }
+  let(:octocat) { OpenStruct.new(id: 123456, login: 'octocat') }
+  let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
+  let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
+  let(:base_data) do
+    {
+      state: 'open',
+      title: 'New feature',
+      body: 'Please pull these awesome changes',
+      head: source_branch,
+      base: target_branch,
+      assignee: nil,
+      user: octocat,
+      created_at: created_at,
+      updated_at: updated_at,
+      closed_at: nil,
+      merged_at: nil
+    }
+  end
+
+  subject(:pull_request) { described_class.new(project, raw_data)}
+
+  describe '#attributes' do
+    context 'when pull request is open' do
+      let(:raw_data) { OpenStruct.new(base_data.merge(state: 'open')) }
+
+      it 'returns formatted attributes' do
+        expected = {
+          title: 'New feature',
+          description: "*Created by: octocat*\n\nPlease pull these awesome changes",
+          source_project: project,
+          source_branch: 'feature',
+          target_project: project,
+          target_branch: 'master',
+          state: 'opened',
+          author_id: project.creator_id,
+          assignee_id: nil,
+          created_at: created_at,
+          updated_at: updated_at
+        }
+
+        expect(pull_request.attributes).to eq(expected)
+      end
+    end
+
+    context 'when pull request is closed' do
+      let(:closed_at) { DateTime.strptime('2011-01-28T19:01:12Z') }
+      let(:raw_data) { OpenStruct.new(base_data.merge(state: 'closed', closed_at: closed_at)) }
+
+      it 'returns formatted attributes' do
+        expected = {
+          title: 'New feature',
+          description: "*Created by: octocat*\n\nPlease pull these awesome changes",
+          source_project: project,
+          source_branch: 'feature',
+          target_project: project,
+          target_branch: 'master',
+          state: 'closed',
+          author_id: project.creator_id,
+          assignee_id: nil,
+          created_at: created_at,
+          updated_at: closed_at
+        }
+
+        expect(pull_request.attributes).to eq(expected)
+      end
+    end
+
+    context 'when pull request is merged' do
+      let(:merged_at) { DateTime.strptime('2011-01-28T13:01:12Z') }
+      let(:raw_data) { OpenStruct.new(base_data.merge(state: 'closed', merged_at: merged_at)) }
+
+      it 'returns formatted attributes' do
+        expected = {
+          title: 'New feature',
+          description: "*Created by: octocat*\n\nPlease pull these awesome changes",
+          source_project: project,
+          source_branch: 'feature',
+          target_project: project,
+          target_branch: 'master',
+          state: 'merged',
+          author_id: project.creator_id,
+          assignee_id: nil,
+          created_at: created_at,
+          updated_at: merged_at
+        }
+
+        expect(pull_request.attributes).to eq(expected)
+      end
+    end
+
+    context 'when it is assigned to someone' do
+      let(:raw_data) { OpenStruct.new(base_data.merge(assignee: octocat)) }
+
+      it 'returns nil as assigned_id when is not a GitLab user' do
+        expect(pull_request.attributes.fetch(:assignee_id)).to be_nil
+      end
+
+      it 'returns GitLab user id as assigned_id when is a GitLab user' do
+        gl_user = create(:omniauth_user, extern_uid: octocat.id, provider: 'github')
+
+        expect(pull_request.attributes.fetch(:assignee_id)).to eq gl_user.id
+      end
+    end
+
+    context 'when author is a GitLab user' do
+      let(:raw_data) { OpenStruct.new(base_data.merge(user: octocat)) }
+
+      it 'returns project#creator_id as author_id when is not a GitLab user' do
+        expect(pull_request.attributes.fetch(:author_id)).to eq project.creator_id
+      end
+
+      it 'returns GitLab user id as author_id when is a GitLab user' do
+        gl_user = create(:omniauth_user, extern_uid: octocat.id, provider: 'github')
+
+        expect(pull_request.attributes.fetch(:author_id)).to eq gl_user.id
+      end
+    end
+  end
+
+  describe '#valid?' do
+    let(:invalid_branch) { OpenStruct.new(ref: 'invalid-branch') }
+
+    context 'when source and target branches exists' do
+      let(:raw_data) { OpenStruct.new(base_data.merge(head: source_branch, base: target_branch)) }
+
+      it 'returns true' do
+        expect(pull_request.valid?).to eq true
+      end
+    end
+
+    context 'when source branch doesn not exists' do
+      let(:raw_data) { OpenStruct.new(base_data.merge(head: invalid_branch, base: target_branch)) }
+
+      it 'returns false' do
+        expect(pull_request.valid?).to eq false
+      end
+    end
+
+    context 'when target branch doesn not exists' do
+      let(:raw_data) { OpenStruct.new(base_data.merge(head: source_branch, base: invalid_branch)) }
+
+      it 'returns false' do
+        expect(pull_request.valid?).to eq false
+      end
+    end
+  end
+end