Merge branch '13426-backfill-version-data' into 'master'

Capture author and created_at data for Version

See merge request gitlab-org/gitlab!17322

db/post_migrate/20191030223057_backfill_version_author_and_created_at.rb

+# frozen_string_literal: true
+
+class BackfillVersionAuthorAndCreatedAt < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+  MIGRATION = 'BackfillVersionDataFromGitaly'.freeze
+  BATCH_SIZE = 500
+
+  disable_ddl_transaction!
+
+  class Project < ActiveRecord::Base
+    self.table_name = 'projects'
+    self.inheritance_column = :_type_disabled
+  end
+
+  class Issue < ActiveRecord::Base
+    self.table_name = 'issues'
+    self.inheritance_column = :_type_disabled
+  end
+
+  class Version < ActiveRecord::Base
+    include EachBatch
+    self.table_name = 'design_management_versions'
+    self.inheritance_column = :_type_disabled
+
+    # Returns unique issue ids of versions that are not in projects
+    # that are pending deletion.
+    scope :with_unique_issue_ids, -> do
+      versions = Version.arel_table
+      issues = Issue.arel_table
+      projects = Project.arel_table
+
+      Version.select(versions[:issue_id]).where(
+        versions[:author_id].eq(nil).or(
+          versions[:created_at].eq(nil)
+        ).and(
+          issues[:project_id].not_in(
+            projects.project(projects[:id]).where(projects[:pending_delete].eq(true))
+          )
+        )
+      ).joins(
+        versions.join(issues).on(
+          issues[:id].eq(versions[:issue_id])
+        ).join_sources
+      ).distinct
+    end
+  end
+
+  # This migration will make around ~1300 UPDATE queries on GitLab.com,
+  # one per design_management_versions record as the migration will update
+  # each record individually.
+  #
+  # It will make around 870 Gitaly `ListCommitsByOid` requests on GitLab.com.
+  # One for every unique issue with design_management_versions records.
+  def up
+    return unless Gitlab.ee? # no-op for CE
+
+    Version.with_unique_issue_ids.each_batch(of: BATCH_SIZE) do |versions, index|
+      jobs = versions.map { |version| [MIGRATION, [version.issue_id]] }
+
+      BackgroundMigrationWorker.bulk_perform_async(jobs)
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

ee/app/models/design_management/design_action.rb

@@ -27,7 +27,7 @@ module DesignManagement
 
     # Parameters:
     # - design [DesignManagement::Design]: the design that was changed
-    # - gitaly_action [Symbol]: the action that gitlay performed
+    # - action [Symbol]: the action that gitaly performed
     def initialize(design, action, content = nil)
       @design, @action, @content = design, action, content
       validate!

ee/app/models/design_management/version.rb

@@ -28,6 +28,7 @@ module DesignManagement
     end
 
     belongs_to :issue
+    belongs_to :author, class_name: 'User'
     has_many :actions
     has_many :designs,
              through: :actions,
@@ -38,6 +39,7 @@ module DesignManagement
     validates :designs, presence: true, unless: :importing?
     validates :sha, presence: true
     validates :sha, uniqueness: { case_sensitive: false, scope: :issue_id }
+    validates :author, presence: true
     # We are not validating the issue object as it incurs an extra query to fetch
     # the record from the DB. Instead, we rely on the foreign key constraint to
     # ensure referential integrity.
@@ -61,18 +63,20 @@ module DesignManagement
     # method inserts designs in bulk, rather than one by one.
     #
     # Parameters:
-    # - designs [DesignManagement::DesignAction]:
+    # - design_actions [DesignManagement::DesignAction]:
     #     the actions that have been performed in the repository.
     # - sha [String]:
     #     the SHA of the commit that performed them
+    # - author [User]:
+    #     the user who performed the commit
     # returns [DesignManagement::Version]
-    def self.create_for_designs(design_actions, sha)
+    def self.create_for_designs(design_actions, sha, author)
       issue_id, not_uniq = design_actions.map(&:issue_id).compact.uniq
       raise NotSameIssue, 'All designs must belong to the same issue!' if not_uniq
 
       transaction do
-        version = safe_find_or_create_by(sha: sha, issue_id: issue_id)
-        version.save(validate: false) # We need it to have an ID, validate later
+        version = new(sha: sha, issue_id: issue_id, author: author)
+        version.save(validate: false) # We need it to have an ID. Validate later when designs are present
 
         rows = design_actions.map { |action| action.row_attrs(version) }
 
@@ -95,11 +99,15 @@ module DesignManagement
     end
 
     def author
-      commit&.author
+      super || (commit_author if persisted?)
     end
 
     private
 
+    def commit_author
+      commit&.author
+    end
+
     def commit
       @commit ||= issue.project.design_repository.commit(sha)
     end

ee/app/services/design_management/runs_design_actions.rb

@@ -17,7 +17,7 @@ module DesignManagement
                                     message: commit_message,
                                     actions: actions.map(&:gitaly_action))
 
-      ::DesignManagement::Version.create_for_designs(actions, sha)
+      ::DesignManagement::Version.create_for_designs(actions, sha, current_user)
     end
   end
 end

ee/changelogs/unreleased/13426-backfill-version-data.yml

+---
+title: Store and look up design management version authorship from database
+merge_request: 17322
+author:
+type: other

ee/lib/gitlab/background_migration/backfill_version_data_from_gitaly.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    class BackfillVersionDataFromGitaly
+      class Version < ActiveRecord::Base
+        self.table_name = 'design_management_versions'
+        self.inheritance_column = :_type_disabled
+
+        # The `sha` of a version record must be deserialized from binary
+        # in order to convert it to a `sha` String that can be used to fetch
+        # a corresponding Commit from Git.
+        def sha
+          value = super
+
+          value.unpack1('H*')
+        end
+
+        scope :backfillable_for_issue, -> (issue_id) do
+          where(author_id: nil).or(where(created_at: nil))
+          .where(issue_id: issue_id)
+        end
+      end
+
+      class Issue < ActiveRecord::Base
+        self.table_name = 'issues'
+        self.inheritance_column = :_type_disabled
+      end
+
+      def perform(issue_id)
+        issue = Issue.find_by_id(issue_id)
+        return unless issue
+
+        # We need a full Project instance in order to initialize a
+        # Repository instance that can perform Gitaly calls.
+        project = Project.find_by_id(issue.project_id)
+        return if project.nil? || project.pending_delete?
+
+        # We need a full Repository instance to perform Gitaly calls.
+        repository = ::DesignManagement::Repository.new(project)
+        versions = Version.backfillable_for_issue(issue_id)
+        commits = commits_for_versions(versions, repository)
+
+        ActiveRecord::Base.transaction do
+          versions.each do |version|
+            commit = commits[version.sha]
+
+            unless commit.nil?
+              version.update_columns(created_at: commit.created_at, author_id: commit.author&.id)
+            end
+          end
+        end
+      end
+
+      private
+
+      # Performs a Gitaly request to fetch the corresponding Commit data
+      # for the given versions.
+      #
+      # Returns Commits as a Hash of { sha => Commit }
+      def commits_for_versions(versions, repository)
+        shas = versions.map(&:sha)
+
+        commits = repository.commits_by(oids: shas)
+        # Batch load the commit authors so the `User` records are fetched
+        # all at once the first time we call `commit.author.id`.
+        commits.each(&:lazy_author)
+
+        commits.each_with_object({}) do |commit, hash|
+          hash[commit.id] = commit
+        end
+      end
+    end
+  end
+end

ee/spec/factories/design_management/designs.rb

@@ -6,9 +6,14 @@ FactoryBot.define do
     project { issue.project }
     sequence(:filename) { |n| "homescreen-#{n}.jpg" }
 
+    transient do
+      author { issue.author }
+    end
+
     create_versions = ->(design, evaluator, commit_version) do
       unless evaluator.versions_count.zero?
         project = design.project
+        issue = design.issue
         repository = project.design_repository
         repository.create_if_not_exists
         dv_table_name = DesignManagement::Action.table_name
@@ -16,7 +21,7 @@ FactoryBot.define do
 
         run_action = ->(action) do
           sha = commit_version[action]
-          version = DesignManagement::Version.new(sha: sha, issue: design.issue)
+          version = DesignManagement::Version.new(sha: sha, issue: issue, author: evaluator.author)
           version.save(validate: false) # We need it to have an ID, validate later
           Gitlab::Database.bulk_insert(dv_table_name, [action.row_attrs(version)])
         end
@@ -78,7 +83,7 @@ FactoryBot.define do
 
         commit_version = ->(action) do
           repository.multi_action(
-            project.creator,
+            evaluator.author,
             branch_name: 'master',
             message: "#{action.action} for #{design.filename}",
             actions: [action.gitaly_action]

ee/spec/factories/design_management/versions.rb

@@ -4,6 +4,7 @@ FactoryBot.define do
   factory :design_version, class: DesignManagement::Version do
     sequence(:sha) { |n| Digest::SHA1.hexdigest("commit-like-#{n}") }
     issue { designs.first&.issue || create(:issue) }
+    author { issue.author || create(:user) }
 
     transient do
       designs_count { 1 }
@@ -58,10 +59,8 @@ FactoryBot.define do
     end
 
     # This trait is for versions that must be present in the git repository.
-    # This might be needed if, for instance, you need to make use of Version#author
     trait :committed do
       transient do
-        author { create(:user) }
         file { File.join(Rails.root, 'spec/fixtures/dk.png') }
       end
 

ee/spec/fixtures/lib/gitlab/import_export/designs/project.json

@@ -129,6 +129,7 @@
                "id":24,
                "sha":"9358d1bac8ff300d3d2597adaa2572a20f7f8703",
                "issue_id":469,
+               "author_id":1,
                "actions":[
                   {
                      "design_id":38,
@@ -147,11 +148,12 @@
                "id":25,
                "sha":"e1a4a501bcb42f291f84e5d04c8f927821542fb6",
                "issue_id":469,
+               "author_id":2,
                "actions":[
                   {
                      "design_id":38,
                      "version_id":25,
-                     "event":0,
+                     "event":1,
                      "design":{
                         "id":38,
                         "project_id":30,
@@ -176,11 +178,12 @@
                "id":26,
                "sha":"27702d08f5ee021ae938737f84e8fe7c38599e85",
                "issue_id":469,
+               "author_id":1,
                "actions":[
                   {
                      "design_id":38,
                      "version_id":26,
-                     "event":0,
+                     "event":1,
                      "design":{
                         "id":38,
                         "project_id":30,
@@ -191,7 +194,7 @@
                   {
                      "design_id":39,
                      "version_id":26,
-                     "event":0,
+                     "event":2,
                      "design":{
                         "id":39,
                         "project_id":30,
@@ -309,6 +312,7 @@
                "id":27,
                "sha":"8587e78ab6bda3bc820a9f014c3be4a21ad4fcc8",
                "issue_id":470,
+               "author_id":1,
                "actions":[
                   {
                      "design_id":41,
@@ -327,6 +331,7 @@
                "id":28,
                "sha":"73f871b4c8c1d65c62c460635e023179fb53abc4",
                "issue_id":470,
+               "author_id":2,
                "actions":[
                   {
                      "design_id":42,
@@ -356,11 +361,12 @@
                "id":29,
                "sha":"c9b5f067f3e892122a4b12b0a25a8089192f3ac8",
                "issue_id":470,
+               "author_id":2,
                "actions":[
                   {
                      "design_id":42,
                      "version_id":29,
-                     "event":0,
+                     "event":1,
                      "design":{
                         "id":42,
                         "project_id":30,
@@ -413,6 +419,29 @@
             "email":"admin@example.com",
             "username":"root"
          }
+      },
+      {
+         "id":96,
+         "access_level":40,
+         "source_id":30,
+         "source_type":"Project",
+         "user_id":2,
+         "notification_level":3,
+         "created_at":"2019-08-07T03:57:32.825Z",
+         "updated_at":"2019-08-07T03:57:32.825Z",
+         "created_by_id":null,
+         "invite_email":null,
+         "invite_token":null,
+         "invite_accepted_at":null,
+         "requested_at":null,
+         "expires_at":null,
+         "ldap":false,
+         "override":false,
+         "user":{
+            "id":2,
+            "email":"user_2@example.com",
+            "username":"user_2"
+         }
       }
    ],
    "merge_requests":[

ee/spec/lib/ee/gitlab/import_export/project_tree_restorer_spec.rb

@@ -5,8 +5,9 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
   include ImportExport::CommonUtil
   using RSpec::Parameterized::TableSyntax
 
-  set(:user) { create(:user) }
-  set(:project) { create(:project, :builds_disabled, :issues_disabled, name: 'project', path: 'project') }
+  let_it_be(:project) { create(:project, :builds_disabled, :issues_disabled, name: 'project', path: 'project') }
+  let_it_be(:user) { create(:admin, username: 'user_1') }
+  let_it_be(:second_user) { create(:user, username: 'user_2' )}
   let(:shared) { project.import_export_shared }
   let(:project_tree_restorer) { described_class.new(user: user, shared: shared, project: project) }
   let(:restored_project_json) { project_tree_restorer.restore }
@@ -29,15 +30,17 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
     describe 'restores issue associations correctly' do
       let(:issue) { project.issues.offset(index).first }
 
-      where(:index, :design_filenames, :version_shas) do
-        0 | %w[chirrido3.jpg jonathan_richman.jpg mariavontrap.jpeg] | %w[27702d08f5ee021ae938737f84e8fe7c38599e85 9358d1bac8ff300d3d2597adaa2572a20f7f8703 e1a4a501bcb42f291f84e5d04c8f927821542fb6]
-        1 | ['1 (1).jpeg', '2099743.jpg', 'a screenshot (1).jpg', 'chirrido3.jpg'] | %w[73f871b4c8c1d65c62c460635e023179fb53abc4 8587e78ab6bda3bc820a9f014c3be4a21ad4fcc8 c9b5f067f3e892122a4b12b0a25a8089192f3ac8]
+      where(:index, :design_filenames, :version_shas, :events, :author_usernames) do
+        0 | %w[chirrido3.jpg jonathan_richman.jpg mariavontrap.jpeg] | %w[27702d08f5ee021ae938737f84e8fe7c38599e85 9358d1bac8ff300d3d2597adaa2572a20f7f8703 e1a4a501bcb42f291f84e5d04c8f927821542fb6] | %w[creation creation creation modification modification deletion] | %w[user_1 user_1 user_2]
+        1 | ['1 (1).jpeg', '2099743.jpg', 'a screenshot (1).jpg', 'chirrido3.jpg'] | %w[73f871b4c8c1d65c62c460635e023179fb53abc4 8587e78ab6bda3bc820a9f014c3be4a21ad4fcc8 c9b5f067f3e892122a4b12b0a25a8089192f3ac8] | %w[creation creation creation creation modification] | %w[user_1 user_2 user_2]
       end
 
       with_them do
         it do
           expect(issue.designs.pluck(:filename)).to contain_exactly(*design_filenames)
           expect(issue.design_versions.pluck(:sha)).to contain_exactly(*version_shas)
+          expect(issue.design_versions.flat_map(&:actions).map(&:event)).to contain_exactly(*events)
+          expect(issue.design_versions.map(&:author).map(&:username)).to contain_exactly(*author_usernames)
         end
       end
     end

ee/spec/lib/ee/gitlab/import_export/project_tree_saver_spec.rb

@@ -43,6 +43,9 @@ describe Gitlab::ImportExport::ProjectTreeSaver do
         end.flatten
 
         expect(issue_json['design_versions'].size).to eq(2)
+        issue_json['design_versions'].each do |version|
+          expect(version['author_id']).to eq(issue.author_id)
+        end
         expect(actions.size).to eq(2)
         actions.each do |action|
           expect(action['design']).to be_present

ee/spec/lib/gitlab/background_migration/backfill_version_data_from_gitaly_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# rubocop:disable RSpec/FactoriesInMigrationSpecs
+describe Gitlab::BackgroundMigration::BackfillVersionDataFromGitaly do
+  let_it_be(:issue) { create(:issue) }
+
+  def perform_worker
+    described_class.new.perform(issue.id)
+  end
+
+  def create_version(attrs)
+    # Use the `:design` factory to create a version that has a
+    # correponding Git commit.
+    attrs[:issue] ||= issue
+    design = create(:design, :with_file, attrs)
+    design.versions.first
+  end
+
+  def create_version_with_missing_data(attrs = {})
+    version = create_version(attrs)
+    version.update_columns(author_id: nil, created_at: nil)
+    version
+  end
+
+  it 'correctly sets version author_id and created_at properties from the Git commit' do
+    version = create_version_with_missing_data
+    commit = issue.project.design_repository.commit(version.sha)
+
+    expect(version).to have_attributes(
+      author_id: nil,
+      created_at: nil
+    )
+    expect(commit.author.id).to be_present
+    expect(commit.created_at).to be_present
+
+    expect { perform_worker }.to(
+      change do
+        version.reload
+        [version.author_id, version.created_at]
+      end
+      .from([nil, nil])
+      .to([commit.author.id, commit.created_at])
+    )
+  end
+
+  it 'avoids N+1 issues and fetches all User records in one call' do
+    author_1, author_2, author_3 = create_list(:user, 3)
+    create_version_with_missing_data(author: author_1)
+    create_version_with_missing_data(author: author_2)
+    create_version_with_missing_data(author: author_3)
+
+    expect(User).to receive(:by_any_email).with(
+      array_including(author_1.email, author_2.email, author_3.email),
+      confirmed: true
+    ).once.and_call_original
+
+    perform_worker
+  end
+
+  it 'leaves versions in a valid state' do
+    version = create_version_with_missing_data
+
+    expect(version).to be_valid
+    expect { perform_worker }.not_to change { version.reload.valid? }
+  end
+
+  it 'skips versions that are in projects that are pending deletion' do
+    version = create_version_with_missing_data
+    version.issue.project.update!(pending_delete: true)
+
+    expect { perform_worker }.not_to(
+      change do
+        version.reload
+        [version.author_id, version.created_at]
+      end
+    )
+  end
+end
+# rubocop:enable RSpec/FactoriesInMigrationSpecs

ee/spec/migrations/backfill_version_author_and_created_at_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20191030223057_backfill_version_author_and_created_at.rb')
+
+describe BackfillVersionAuthorAndCreatedAt, :migration, :sidekiq do
+  let_it_be(:migration_name) { described_class::MIGRATION.to_s.demodulize }
+  let_it_be(:projects) { table(:projects) }
+  let_it_be(:issues) { table(:issues) }
+  let_it_be(:versions) { table(:design_management_versions) }
+  let_it_be(:users) { table(:users) }
+  let(:project) { projects.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce', namespace_id: 1) }
+  let(:issue_1) { create_issue }
+  let(:issue_2) { create_issue }
+  let(:issue_3) { create_issue }
+  let(:author) { users.create!(email: 'email@email.com', name: 'foo', username: 'foo', projects_limit: 0) }
+
+  def create_issue
+    issues.create!(project_id: project.id)
+  end
+
+  def create_version(attrs = {})
+    unless attrs[:issue_id]
+      issue = create_issue
+      attrs[:issue_id] = issue.id
+    end
+
+    versions.create!(attrs)
+  end
+
+  describe 'scheduling' do
+    it 'schedules background migrations in bulk, one job per unique issue id' do
+      create_version(sha: 'foo', issue_id: issue_1.id)
+      create_version(sha: 'bar', issue_id: issue_1.id)
+      create_version(sha: 'baz', issue_id: issue_2.id)
+
+      Sidekiq::Testing.fake! do
+        expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with(
+          [[migration_name, [issue_1.id]], [migration_name, [issue_2.id]]]
+        )
+
+        migrate!
+      end
+    end
+
+    it 'schedules background migrations in batches' do
+      create_version(sha: 'foo', issue_id: issue_1.id)
+      create_version(sha: 'bar', issue_id: issue_2.id)
+      create_version(sha: 'baz', issue_id: issue_3.id)
+
+      stub_const("#{described_class.name}::BATCH_SIZE", 2)
+
+      Sidekiq::Testing.fake! do
+        # First batch
+        expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with(
+          [[migration_name, [issue_1.id]], [migration_name, [issue_2.id]]]
+        )
+        # Second batch
+        expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with(
+          [[migration_name, [issue_3.id]]]
+        )
+
+        migrate!
+      end
+    end
+  end
+
+  describe 'scoping version records' do
+    it 'schedules background migrations for versions that have NULL author_ids' do
+      version = create_version(sha: 'foo', created_at: Time.now)
+
+      expect(version.author_id).to be_nil
+
+      Sidekiq::Testing.fake! do
+        expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with(
+          [[migration_name, [version.issue_id]]]
+        )
+
+        migrate!
+      end
+    end
+
+    it 'schedules background migrations for versions that have NULL created_ats' do
+      version = create_version(sha: 'foo', author_id: author.id)
+      # We can't create a record with NULL created_at within this migration
+      # so update it here to be NULL.
+      version.update!(created_at: nil)
+
+      expect(version.created_at).to be_nil
+
+      Sidekiq::Testing.fake! do
+        expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with(
+          [[migration_name, [version.issue_id]]]
+        )
+
+        migrate!
+      end
+    end
+
+    it 'does not schedule background migrations for versions that have author_ids and created_ats' do
+      create_version(sha: 'foo', author_id: author.id, created_at: Time.now)
+
+      Sidekiq::Testing.fake! do
+        expect(BackgroundMigrationWorker).not_to receive(:bulk_perform_async)
+
+        migrate!
+      end
+    end
+
+    it 'does not schedule background migrations for versions that are in projects that are pending deletion' do
+      project.update!(pending_delete: true)
+      create_version(sha: 'foo')
+
+      Sidekiq::Testing.fake! do
+        expect(BackgroundMigrationWorker).not_to receive(:bulk_perform_async)
+
+        migrate!
+      end
+    end
+  end
+end

ee/spec/models/design_management/version_spec.rb

@@ -26,6 +26,7 @@ describe DesignManagement::Version do
     subject(:design_version) { build(:design_version) }
 
     it { is_expected.to be_valid }
+    it { is_expected.to validate_presence_of(:author) }
     it { is_expected.to validate_presence_of(:sha) }
     it { is_expected.to validate_presence_of(:designs) }
     it { is_expected.to validate_presence_of(:issue_id) }
@@ -33,8 +34,8 @@ describe DesignManagement::Version do
   end
 
   describe "scopes" do
-    let(:version_1) { create(:design_version) }
-    let(:version_2) { create(:design_version) }
+    let_it_be(:version_1) { create(:design_version) }
+    let_it_be(:version_2) { create(:design_version) }
 
     describe ".for_designs" do
       it "only returns versions related to the specified designs" do
@@ -72,16 +73,17 @@ describe DesignManagement::Version do
       end
     end
 
-    set(:issue) { create(:issue) }
-    set(:design_a) { create(:design, issue: issue) }
-    set(:design_b) { create(:design, issue: issue) }
-    let(:designs) { [design_a, design_b] }
+    let_it_be(:author) { create(:user) }
+    let_it_be(:issue) { create(:issue) }
+    let_it_be(:design_a) { create(:design, issue: issue) }
+    let_it_be(:design_b) { create(:design, issue: issue) }
+    let_it_be(:designs) { [design_a, design_b] }
 
     describe 'the error raised when there are no actions' do
-      let(:sha) { 'f00' }
+      let_it_be(:sha) { 'f00' }
 
       def call_with_empty_actions
-        described_class.create_for_designs([], sha)
+        described_class.create_for_designs([], sha, author)
       end
 
       it 'raises CouldNotCreateVersion' do
@@ -103,12 +105,12 @@ describe DesignManagement::Version do
     end
 
     describe 'the error raised when the designs come from different issues' do
-      let(:sha) { 'f00' }
-      let(:designs) { create_list(:design, 2) }
-      let(:actions) { as_actions(designs) }
+      let_it_be(:sha) { 'f00' }
+      let_it_be(:designs) { create_list(:design, 2) }
+      let_it_be(:actions) { as_actions(designs) }
 
       def call_with_mismatched_designs
-        described_class.create_for_designs(actions, sha)
+        described_class.create_for_designs(actions, sha, author)
       end
 
       it 'raises CouldNotCreateVersion' do
@@ -131,21 +133,21 @@ describe DesignManagement::Version do
 
     it 'does not leave invalid versions around if creation fails' do
       expect do
-        described_class.create_for_designs([], 'abcdef') rescue nil
+        described_class.create_for_designs([], 'abcdef', author) rescue nil
       end.not_to change { described_class.count }
     end
 
     it 'does not leave orphaned design-versions around if creation fails' do
       actions = as_actions(designs)
       expect do
-        described_class.create_for_designs(actions, '') rescue nil
+        described_class.create_for_designs(actions, '', author) rescue nil
       end.not_to change { DesignManagement::Action.count }
     end
 
-    it "creates a version and links it to multiple designs" do
+    it 'creates a version and links it to multiple designs' do
       actions = as_actions(designs, :create)
 
-      version = described_class.create_for_designs(actions, "abc")
+      version = described_class.create_for_designs(actions, 'abc', author)
 
       expect(version.designs).to contain_exactly(*designs)
       expect(designs.map(&method(:current_version_id))).to all(eq version.id)
@@ -154,7 +156,7 @@ describe DesignManagement::Version do
     it 'creates designs if they are new to git' do
       actions = as_actions(designs, :create)
 
-      described_class.create_for_designs(actions, "abc")
+      described_class.create_for_designs(actions, 'abc', author)
 
       expect(designs.map(&:most_recent_action)).to all(be_creation)
     end
@@ -162,15 +164,23 @@ describe DesignManagement::Version do
     it 'correctly associates the version with the issue' do
       actions = as_actions(designs)
 
-      version = described_class.create_for_designs(actions, "abc")
+      version = described_class.create_for_designs(actions, 'abc', author)
 
       expect(version.issue).to eq(issue)
     end
 
+    it 'correctly associates the version with the author' do
+      actions = as_actions(designs)
+
+      version = described_class.create_for_designs(actions, 'abc', author)
+
+      expect(version.author).to eq(author)
+    end
+
     it 'modifies designs if git updated them' do
       actions = as_actions(designs, :update)
 
-      described_class.create_for_designs(actions, "abc")
+      described_class.create_for_designs(actions, 'abc', author)
 
       expect(designs.map(&:most_recent_action)).to all(be_modification)
     end
@@ -178,18 +188,18 @@ describe DesignManagement::Version do
     it 'deletes designs when the git action was delete' do
       actions = as_actions(designs, :delete)
 
-      described_class.create_for_designs(actions, "def")
+      described_class.create_for_designs(actions, 'def', author)
 
       expect(designs).to all(be_deleted)
     end
 
     it 're-creates designs if they are deleted' do
-      described_class.create_for_designs(as_actions(designs, :create), "abc")
-      described_class.create_for_designs(as_actions(designs, :delete), "def")
+      described_class.create_for_designs(as_actions(designs, :create), 'abc', author)
+      described_class.create_for_designs(as_actions(designs, :delete), 'def', author)
 
       expect(designs).to all(be_deleted)
 
-      described_class.create_for_designs(as_actions(designs, :create), "ghi")
+      described_class.create_for_designs(as_actions(designs, :create), 'ghi', author)
 
       expect(designs.map(&:most_recent_action)).to all(be_creation)
       expect(designs).not_to include(be_deleted)
@@ -197,28 +207,20 @@ describe DesignManagement::Version do
 
     it 'changes the version of the designs' do
       actions = as_actions([design_a])
-      described_class.create_for_designs(actions, "before")
+      described_class.create_for_designs(actions, 'before', author)
 
       expect do
-        described_class.create_for_designs(actions, "after")
+        described_class.create_for_designs(actions, 'after', author)
       end.to change { current_version_id(design_a) }
     end
   end
 
-  describe '#author' do
-    let(:author) { create(:user) }
-
-    subject(:version) { create(:design_version, :committed, author: author) }
-
-    it { is_expected.to have_attributes(author: author) }
-  end
-
   describe '#designs_by_event' do
     context 'there is a single design' do
-      set(:design) { create(:design) }
+      let_it_be(:design) { create(:design) }
 
       shared_examples :a_correctly_categorised_design do |kind, category|
-        let(:version) { create(:design_version, kind => [design]) }
+        let_it_be(:version) { create(:design_version, kind => [design]) }
 
         it 'returns a hash with a single key and the single design in that bucket' do
           expect(version.designs_by_event).to eq(category => [design])
@@ -231,7 +233,7 @@ describe DesignManagement::Version do
     end
 
     context 'there are a bunch of different designs in a variety of states' do
-      let(:version) do
+      let_it_be(:version) do
         create(:design_version,
                created_designs: create_list(:design, 3),
                modified_designs: create_list(:design, 4),
@@ -254,4 +256,31 @@ describe DesignManagement::Version do
       end
     end
   end
+
+  describe '#author' do
+    it 'returns the author' do
+      author = build(:user)
+      version = build(:design_version, author: author)
+
+      expect(version.author).to eq(author)
+    end
+
+    it 'returns nil if author_id is nil and version is not persisted' do
+      version = build(:design_version, author: nil)
+
+      expect(version.author).to eq(nil)
+    end
+
+    it 'retrieves author from the Commit if author_id is nil and version has been persisted' do
+      author = create(:user)
+      version = create(:design_version, :committed, author: author)
+      author.destroy
+      version.reload
+      commit = version.issue.project.design_repository.commit(version.sha)
+      commit_user = create(:user, email: commit.author_email, name: commit.author_name)
+
+      expect(version.author_id).to eq(nil)
+      expect(version.author).to eq(commit_user)
+    end
+  end
 end

ee/spec/services/design_management/delete_designs_service_spec.rb

@@ -37,6 +37,12 @@ describe DesignManagement::DeleteDesignsService do
     it 'returns successfully', :aggregate_failures do
       expect(response).to include(status: :success)
     end
+
+    it 'saves the user as the author' do
+      version = response[:version]
+
+      expect(version.author).to eq(user)
+    end
   end
 
   before do

ee/spec/services/design_management/save_designs_service_spec.rb

@@ -131,6 +131,12 @@ describe DesignManagement::SaveDesignsService do
           expect(updated_designs.first.versions.size).to eq(1)
         end
 
+        it 'saves the user as the author' do
+          updated_designs = response[:designs]
+
+          expect(updated_designs.first.versions.first.author).to eq(user)
+        end
+
         describe 'saving the file to LFS' do
           before do
             expect_next_instance_of(Lfs::FileTransformer) do |transformer|

ee/spec/services/system_note_service_spec.rb

@@ -53,18 +53,12 @@ describe SystemNoteService do
     subject { described_class.design_version_added(version) }
 
     # default (valid) parameters:
-    let(:n_designs) { 3 }
-    let(:designs) { create_list(:design, n_designs, issue: issue) }
-    let(:user) { build(:user) }
-    let(:version) do
+    let_it_be(:n_designs) { 3 }
+    let_it_be(:designs) { create_list(:design, n_designs, issue: issue) }
+    let_it_be(:version) do
       create(:design_version, issue: issue, designs: designs)
     end
 
-    before do
-      # Avoid needing to call into gitaly
-      allow(version).to receive(:author).and_return(user)
-    end
-
     context 'with one kind of event' do
       before do
         DesignManagement::Action
@@ -81,7 +75,7 @@ describe SystemNoteService do
     end
 
     context 'with a mixture of events' do
-      let(:n_designs) { DesignManagement::Action.events.size }
+      let_it_be(:n_designs) { DesignManagement::Action.events.size }
 
       before do
         designs.each_with_index do |design, i|

lib/gitlab/import_export/import_export.yml

@@ -272,7 +272,7 @@ ee:
               - :push_event_payload
         - design_versions:
           - actions:
-            - :design # Duplicate export of issues.designs in order to link the record to both Issue and DesignVersion
+            - :design # Duplicate export of issues.designs in order to link the record to both Issue and Action
       - protected_branches:
         - :unprotect_access_levels
       - protected_environments:

spec/lib/gitlab/import_export/all_models.yml

@@ -549,6 +549,7 @@ actions:
 - design
 - version
 versions: &version
+- author
 - issue
 - designs
 - actions