started refactoring some stuff based on MR feedback

app/models/concerns/importable.rb

@@ -0,0 +1,6 @@
+module Importable
+  extend ActiveSupport::Concern
+
+  attr_accessor :importing
+  alias_method :importing?, :importing
+end

app/models/member.rb

@@ -19,10 +19,10 @@
 
 class Member < ActiveRecord::Base
   include Sortable
+  include Importable
   include Gitlab::Access
 
   attr_accessor :raw_invite_token
-  attr_accessor :importing
 
   belongs_to :created_by, class_name: "User"
   belongs_to :user
@@ -55,10 +55,10 @@ class Member < ActiveRecord::Base
   scope :owners,  -> { where(access_level: OWNER) }
 
   before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? }
-  after_create :send_invite, if: :invite?, unless: :importing
+  after_create :send_invite, if: :invite?, unless: :importing?
-  after_create :create_notification_setting, unless: [:invite?, :importing]
+  after_create :create_notification_setting, unless: [:invite?, :importing?]
-  after_create :post_create_hook, unless: [:invite?, :importing]
+  after_create :post_create_hook, unless: [:invite?, :importing?]
-  after_update :post_update_hook, unless: [:invite?, :importing]
+  after_update :post_update_hook, unless: [:invite?, :importing?]
   after_destroy :post_destroy_hook, unless: :invite?
 
   delegate :name, :username, :email, to: :user, prefix: true

app/models/merge_request.rb

@@ -34,6 +34,7 @@ class MergeRequest < ActiveRecord::Base
   include Referable
   include Sortable
   include Taskable
+  include Importable
 
   belongs_to :target_project, foreign_key: :target_project_id, class_name: "Project"
   belongs_to :source_project, foreign_key: :source_project_id, class_name: "Project"
@@ -48,8 +49,6 @@ class MergeRequest < ActiveRecord::Base
 
   delegate :commits, :diffs, :real_size, to: :merge_request_diff, prefix: nil
 
-  attr_accessor :importing
-
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
   attr_accessor :allow_broken
@@ -123,12 +122,12 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  validates :source_project, presence: true, unless: [:allow_broken, :importing]
+  validates :source_project, presence: true, unless: [:allow_broken, :importing?]
   validates :source_branch, presence: true
   validates :target_project, presence: true
   validates :target_branch, presence: true
   validates :merge_user, presence: true, if: :merge_when_build_succeeds?
-  validate :validate_branches, unless: [:allow_broken, :importing]
+  validate :validate_branches, unless: [:allow_broken, :importing?]
   validate :validate_fork
 
   scope :by_branch, ->(branch_name) { where("(source_branch LIKE :branch) OR (target_branch LIKE :branch)", branch: branch_name) }

app/models/merge_request_diff.rb

@@ -15,6 +15,7 @@
 
 class MergeRequestDiff < ActiveRecord::Base
   include Sortable
+  include Importable
 
   # Prevent store of diff if commits amount more then 500
   COMMITS_SAFE_SIZE = 100
@@ -37,9 +38,7 @@ class MergeRequestDiff < ActiveRecord::Base
   serialize :st_commits
   serialize :st_diffs
 
-  after_create :reload_content, unless: :importing
+  after_create :reload_content, unless: :importing?
-
-  attr_accessor :importing
 
   def reload_content
     reload_commits

db/schema.rb

@@ -70,11 +70,11 @@ ActiveRecord::Schema.define(version: 20160508194200) do
     t.string   "recaptcha_site_key"
     t.string   "recaptcha_private_key"
     t.integer  "metrics_port",                      default: 8089
-    t.boolean  "akismet_enabled",                   default: false
-    t.string   "akismet_api_key"
     t.integer  "metrics_sample_interval",           default: 15
     t.boolean  "sentry_enabled",                    default: false
     t.string   "sentry_dsn"
+    t.boolean  "akismet_enabled",                   default: false
+    t.string   "akismet_api_key"
     t.boolean  "email_author_in_body",              default: false
     t.integer  "default_group_visibility"
     t.boolean  "repository_checks_enabled",         default: false
@@ -428,8 +428,8 @@ ActiveRecord::Schema.define(version: 20160508194200) do
     t.integer  "updated_by_id"
     t.boolean  "confidential",  default: false
     t.datetime "deleted_at"
-    t.date     "due_date"
     t.integer  "moved_to_id"
+    t.date     "due_date"
   end
 
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -716,8 +716,8 @@ ActiveRecord::Schema.define(version: 20160508194200) do
     t.integer "project_id"
     t.text    "data"
     t.text    "encrypted_credentials"
-    t.string  "encrypted_credentials_iv"
+    t.text    "encrypted_credentials_iv"
-    t.string  "encrypted_credentials_salt"
+    t.text    "encrypted_credentials_salt"
   end
 
   create_table "projects", force: :cascade do |t|
@@ -815,9 +815,9 @@ ActiveRecord::Schema.define(version: 20160508194200) do
     t.string   "type"
     t.string   "title"
     t.integer  "project_id"
-    t.datetime "created_at"
+    t.datetime "created_at",                               null: false
-    t.datetime "updated_at"
+    t.datetime "updated_at",                               null: false
-    t.boolean  "active",                default: false,    null: false
+    t.boolean  "active",                                   null: false
     t.text     "properties"
     t.boolean  "template",              default: false
     t.boolean  "push_events",           default: true

lib/gitlab/import_export/members_mapper.rb

@@ -2,7 +2,7 @@ module Gitlab
   module ImportExport
     class MembersMapper
 
-      attr_reader :map, :note_member_list
+      attr_reader :note_member_list
 
       def initialize(exported_members:, user:, project:)
         @exported_members = exported_members
@@ -19,33 +19,37 @@ module Gitlab
           default_project_member
         end
 
-        @map = generate_map
       end
 
       def default_project_member
         @default_project_member ||=
           begin
             default_member = ProjectMember.new(default_project_member_hash)
-            default_member.save!
+            default_member.create!
             default_member.user.id
           end
       end
 
+      def map
+        @map ||=
+          begin
+            @exported_members.inject(@project_member_map) do |hash, member|
+              existing_user = User.where(find_project_user_query(member)).first
+              if existing_user
+                old_user_id = member['user']['id']
+                add_user_as_team_member(existing_user, member)
+                hash[old_user_id] = existing_user.id
+              end
+              hash
+            end
+          end
+      end
+
       private
 
-      def generate_map
+      def add_user_as_team_member(existing_user, member)
-        @exported_members.each do |member|
-          existing_user = User.where(find_project_user_query(member)).first
-          assign_member(existing_user, member) if existing_user
-        end
-        @project_member_map
-      end
-
-      def assign_member(existing_user, member)
-        old_user_id = member['user']['id']
         member['user'] = existing_user
-        project_member = ProjectMember.new(member_hash(member))
+        ProjectMember.create!(member_hash(member))
-        @project_member_map[old_user_id] = project_member.user.id if project_member.save
       end
 
       def member_hash(member)

lib/gitlab/import_export/project_tree_restorer.rb

@@ -32,14 +32,15 @@ module Gitlab
                                                                     project: project)
       end
 
-      def create_relations(relation_list = default_relation_list, tree_hash = @tree_hash)
+      def create_relations
         saved = []
-        relation_list.each do |relation|
+        default_relation_list.each do |relation|
-          next if !relation.is_a?(Hash) && tree_hash[relation.to_s].blank?
+          next unless relation.is_a?(Hash) || @tree_hash[relation.to_s].present?
-          create_sub_relations(relation, tree_hash) if relation.is_a?(Hash)
+
+          create_sub_relations(relation, @tree_hash) if relation.is_a?(Hash)
 
           relation_key = relation.is_a?(Hash) ? relation.keys.first : relation
-          relation_hash = create_relation(relation_key, tree_hash[relation_key.to_s])
+          relation_hash = create_relation(relation_key, @tree_hash[relation_key.to_s])
           saved << project.update_attribute(relation_key, relation_hash)
         end
         saved.all?
@@ -73,32 +74,19 @@ module Gitlab
               relation_hash = relation_item[sub_relation.to_s]
             end
 
-            process_sub_relation(relation_hash, relation_item, sub_relation) unless relation_hash.blank?
+            relation_item[sub_relation.to_s] = create_relation(sub_relation, relation_hash) unless relation_hash.blank?
           end
         end
       end
 
-      def process_sub_relation(relation_hash, relation_item, sub_relation)
-        if relation_hash.is_a?(Array)
-          sub_relation_object = create_relation(sub_relation, relation_hash)
-        else
-          sub_relation_object = relation_from_factory(sub_relation, relation_hash)
-        end
-        relation_item[sub_relation.to_s] = sub_relation_object
-      end
-
       def create_relation(relation, relation_hash_list)
         [relation_hash_list].flatten.map do |relation_hash|
-          relation_from_factory(relation, relation_hash)
+          Gitlab::ImportExport::RelationFactory.create(relation_sym: relation.to_sym,
+                                                       relation_hash: relation_hash.merge('project_id' => project.id),
+                                                       members_mapper: members_mapper,
+                                                       user_admin: @user.is_admin?)
         end
       end
-
-      def relation_from_factory(relation, relation_hash)
-        Gitlab::ImportExport::RelationFactory.create(relation_sym: relation.to_sym,
-                                                     relation_hash: relation_hash.merge('project_id' => project.id),
-                                                     members_mapper: members_mapper,
-                                                     user_admin: @user.is_admin?)
-      end
     end
   end
 end

lib/gitlab/import_export/relation_factory.rb

@@ -11,11 +11,14 @@ module Gitlab
                     builds: 'Ci::Build',
                     hooks: 'ProjectHook' }.freeze
 
-      USER_REFERENCES = %w(author_id assignee_id updated_by_id user_id).freeze
+      USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id].freeze
 
+      # Guesses a model and saves it to the DB given its name `relation_sym`
       def create(relation_sym:, relation_hash:, members_mapper:, user_admin:)
         relation_sym = parse_relation_sym(relation_sym)
-        klass = parse_relation(relation_hash, relation_sym)
+
+        klass = relation_class(relation_sym)
+        relation_hash.delete('id')
 
         update_missing_author(relation_hash, members_mapper, user_admin) if relation_sym == :notes
         update_user_references(relation_hash, members_mapper.map)
@@ -49,8 +52,8 @@ module Gitlab
 
         return unless user_admin && members_map.note_member_list.include?(old_author_id)
 
-        relation_hash['note'] = ('*Blank note*') if relation_hash['note'].blank?
+        relation_hash['note'] = '*Blank note*' if relation_hash['note'].blank?
-        relation_hash['note'] += (missing_author_note(relation_hash['updated_at'], author['name']))
+        relation_hash['note'] += missing_author_note(relation_hash['updated_at'], author['name'])
       end
 
       def missing_author_note(updated_at, author_name)
@@ -109,12 +112,6 @@ module Gitlab
         imported_object.importing = true if imported_object.respond_to?(:importing)
         imported_object
       end
-
-      def parse_relation(relation_hash, relation_sym)
-        klass = relation_class(relation_sym)
-        relation_hash.delete('id')
-        klass
-      end
     end
   end
 end

lib/gitlab/import_export/repo_restorer.rb

@@ -12,7 +12,6 @@ module Gitlab
       def restore
         return true unless File.exists?(@path_to_bundle)
 
-        FileUtils.mkdir_p(repos_path)
         FileUtils.mkdir_p(path_to_repo)
 
         git_unbundle(repo_path: path_to_repo, bundle_path: @path_to_bundle)