Refactor Project#create_or_update_import_data

In https://gitlab.com/gitlab-org/release/framework/issues/28 we found
that this method was changed a lot over the years: 43 times if our
calculations were correct. Looking at the method, it had quite a few
branches going on:

    def create_or_update_import_data(data: nil, credentials: nil)
      return if data.nil? && credentials.nil?

      project_import_data = import_data || build_import_data

      if data
        project_import_data.data ||= {}
        project_import_data.data = project_import_data.data.merge(data)
      end

      if credentials
        project_import_data.credentials ||= {}
        project_import_data.credentials =
          project_import_data.credentials.merge(credentials)
      end

      project_import_data
    end

If we turn the || and ||= operators into regular if statements, we can
see a bit more clearly that this method has quite a lot of branches in
it:

    def create_or_update_import_data(data: nil, credentials: nil)
      if data.nil? && credentials.nil?
        return
      else
        project_import_data =
          if import_data
            import_data
          else
            build_import_data
          end

        if data
          if project_import_data.data
            # nothing
          else
            project_import_data.data = {}
          end

          project_import_data.data =
            project_import_data.data.merge(data)
        end

        if credentials
          if project_import_data.credentials
            # nothing
          else
            project_import_data.credentials = {}
          end

          project_import_data.credentials =
            project_import_data.credentials.merge(credentials)
        end

        project_import_data
      end
    end

The number of if statements and branches here makes it easy to make
mistakes. To resolve this, we refactor this code in such a way that we
can get rid of all but the first `if data.nil? && credentials.nil?`
statement. We can do this by simply sending `to_h` to `nil` in the right
places, which removes the need for statements such as `if data`.

Since this data gets written to a database, in ProjectImportData we do
make sure to not write empty Hash values. This requires an `unless`
(which is really a `if !`), but the resulting code is still very easy to
read.

3 files changed, 52 insertions, 8 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 075c07f5c8e..d64532ae21e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -749,15 +749,9 @@ class Project < ActiveRecord::Base
     return if data.nil? && credentials.nil?
 
     project_import_data = import_data || build_import_data
-    if data
-      project_import_data.data ||= {}
-      project_import_data.data = project_import_data.data.merge(data)
-    end
 
-    if credentials
-      project_import_data.credentials ||= {}
-      project_import_data.credentials = project_import_data.credentials.merge(credentials)
-    end
+    project_import_data.merge_data(data.to_h)
+    project_import_data.merge_credentials(credentials.to_h)
 
     project_import_data
   end
diff --git a/app/models/project_import_data.rb b/app/models/project_import_data.rb
index 2c3080c6d8d..525725034a5 100644
--- a/app/models/project_import_data.rb
+++ b/app/models/project_import_data.rb
@@ -22,4 +22,12 @@ class ProjectImportData < ActiveRecord::Base
     # bang doesn't work here - attr_encrypted makes it not to work
     self.credentials = self.credentials.deep_symbolize_keys unless self.credentials.blank?
   end
+
+  def merge_data(hash)
+    self.data = data.to_h.merge(hash) unless hash.empty?
+  end
+
+  def merge_credentials(hash)
+    self.credentials = credentials.to_h.merge(hash) unless hash.empty?
+  end
 end
diff --git a/spec/models/project_import_data_spec.rb b/spec/models/project_import_data_spec.rb
new file mode 100644
index 00000000000..e9910c0a5d1
--- /dev/null
+++ b/spec/models/project_import_data_spec.rb
@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ProjectImportData do
+  describe '#merge_data' do
+    it 'writes the Hash to the attribute if it is nil' do
+      row = described_class.new
+
+      row.merge_data('number' => 10)
+
+      expect(row.data).to eq({ 'number' => 10 })
+    end
+
+    it 'merges the Hash into an existing Hash if one was present' do
+      row = described_class.new(data: { 'number' => 10 })
+
+      row.merge_data('foo' => 'bar')
+
+      expect(row.data).to eq({ 'number' => 10, 'foo' => 'bar' })
+    end
+  end
+
+  describe '#merge_credentials' do
+    it 'writes the Hash to the attribute if it is nil' do
+      row = described_class.new
+
+      row.merge_credentials('number' => 10)
+
+      expect(row.credentials).to eq({ 'number' => 10 })
+    end
+
+    it 'merges the Hash into an existing Hash if one was present' do
+      row = described_class.new
+
+      row.credentials = { 'number' => 10 }
+      row.merge_credentials('foo' => 'bar')
+
+      expect(row.credentials).to eq({ 'number' => 10, 'foo' => 'bar' })
+    end
+  end
+end