Merge branch '13495-a-design-at-version-model-changes' into 'master'

13495-a: Create DesignAtVersion model

See merge request gitlab-org/gitlab!21119

ee/app/finders/design_management/versions_finder.rb

@@ -24,6 +24,7 @@ module DesignManagement
 
       items = design_or_collection.versions
       items = by_earlier_or_equal_to(items)
+      items = by_sha(items)
       items.ordered
     end
 
@@ -34,5 +35,11 @@ module DesignManagement
 
       items.earlier_or_equal_to(params[:earlier_or_equal_to])
     end
+
+    def by_sha(items)
+      return items unless params[:sha]
+
+      items.by_sha(params[:sha])
+    end
   end
 end

ee/app/models/design_management/design.rb

@@ -107,9 +107,7 @@ module DesignManagement
     end
 
     def diff_refs
-      strong_memoize(:diff_refs) do
-        head_version.presence && repository.commit(head_version.sha).diff_refs
-      end
+      strong_memoize(:diff_refs) { head_version&.diff_refs }
     end
 
     def clear_version_cache
@@ -158,7 +156,7 @@ module DesignManagement
 
     def user_notes_count_service
       strong_memoize(:user_notes_count_service) do
-        DesignManagement::DesignUserNotesCountService.new(self) # rubocop: disable CodeReuse/ServiceClass
+        ::DesignManagement::DesignUserNotesCountService.new(self) # rubocop: disable CodeReuse/ServiceClass
       end
     end
   end

ee/app/models/design_management/design_at_version.rb

+# frozen_string_literal: true
+
+# Tuple of design and version
+# * has a composite ID, with lazy_find
+module DesignManagement
+  class DesignAtVersion
+    include ActiveModel::Validations
+    include GlobalID::Identification
+    include Gitlab::Utils::StrongMemoize
+
+    attr_reader :version
+    attr_reader :design
+
+    validates_presence_of :version
+    validates_presence_of :design
+
+    validate :design_and_version_belong_to_the_same_issue
+    validate :design_and_version_have_issue_id
+
+    def initialize(design: nil, version: nil)
+      @design, @version = design, version
+    end
+
+    def self.instantiate(attrs)
+      new(attrs).tap { |obj| obj.validate! }
+    end
+
+    # The ID, needed by GraphQL types and as part of the Lazy-fetch
+    # protocol, includes information about both the design and the version.
+    #
+    # The particular format is not interesting, and should be treated as opaque
+    # by all callers.
+    def id
+      "#{design.id}.#{version.id}"
+    end
+
+    def self.lazy_find(id)
+      BatchLoader.for(id).batch do |ids, callback|
+        find(ids).each do |record|
+          callback.call(record.id, record)
+        end
+      end
+    end
+
+    def self.find(ids)
+      pairs = ids.map { |id| id.split('.').map(&:to_i) }
+
+      design_ids = pairs.map(&:first).uniq
+      version_ids = pairs.map(&:second).uniq
+
+      designs = ::DesignManagement::Design
+        .where(id: design_ids)
+        .index_by(&:id)
+
+      versions = ::DesignManagement::Version
+        .where(id: version_ids)
+        .index_by(&:id)
+
+      pairs.map do |(design_id, version_id)|
+        design = designs[design_id]
+        version = versions[version_id]
+
+        obj = new(design: design, version: version)
+
+        obj if obj.valid?
+      end.compact
+    end
+
+    def status
+      if not_created_yet?
+        :not_created_yet
+      elsif deleted?
+        :deleted
+      else
+        :current
+      end
+    end
+
+    def deleted?
+      action&.deletion?
+    end
+
+    def not_created_yet?
+      action.nil?
+    end
+
+    private
+
+    def action
+      strong_memoize(:most_recent_action) do
+        ::DesignManagement::Action
+          .most_recent.up_to_version(version)
+          .where(design: design)
+          .first
+      end
+    end
+
+    def design_and_version_belong_to_the_same_issue
+      id_a, id_b = [design, version].map { |obj| obj&.issue_id }
+
+      return if id_a == id_b
+
+      errors.add(:issue, "must be the same on design and version")
+    end
+
+    def design_and_version_have_issue_id
+      return if [design, version].all? { |obj| obj.try(:issue_id).present? }
+
+      errors.add(:issue, "must be present on both design and version")
+    end
+  end
+end

ee/app/models/design_management/version.rb

@@ -4,6 +4,7 @@ module DesignManagement
   class Version < ApplicationRecord
     include Importable
     include ShaAttribute
+    include Gitlab::Utils::StrongMemoize
 
     NotSameIssue = Class.new(StandardError)
 
@@ -50,11 +51,12 @@ module DesignManagement
     delegate :project, to: :issue
 
     scope :for_designs, -> (designs) do
-      where(id: Action.where(design_id: designs).select(:version_id)).distinct
+      where(id: ::DesignManagement::Action.where(design_id: designs).select(:version_id)).distinct
     end
     scope :earlier_or_equal_to, -> (version) { where('id <= ?', version) }
     scope :ordered, -> { order(id: :desc) }
     scope :for_issue, -> (issue) { where(issue: issue) }
+    scope :by_sha, -> (sha) { where(sha: sha) }
 
     # This is the one true way to create a Version.
     #
@@ -80,7 +82,7 @@ module DesignManagement
 
         rows = design_actions.map { |action| action.row_attrs(version) }
 
-        Gitlab::Database.bulk_insert(Action.table_name, rows)
+        Gitlab::Database.bulk_insert(::DesignManagement::Action.table_name, rows)
         version.designs.reset
         version.validate!
         design_actions.each(&:performed)
@@ -102,6 +104,15 @@ module DesignManagement
       super || (commit_author if persisted?)
     end
 
+    def diff_refs
+      strong_memoize(:diff_refs) { commit&.diff_refs }
+    end
+
+    def reset
+      %i[diff_refs commit].each { |k| clear_memoization(k) }
+      super
+    end
+
     private
 
     def commit_author
@@ -109,7 +120,7 @@ module DesignManagement
     end
 
     def commit
-      @commit ||= issue.project.design_repository.commit(sha)
+      strong_memoize(:commit) { issue.project.design_repository.commit(sha) }
     end
   end
 end

ee/app/policies/design_management/design_at_version_policy.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  class DesignAtVersionPolicy < ::BasePolicy
+    delegate { @subject.version }
+    delegate { @subject.design }
+  end
+end

ee/spec/factories/design_management/design_at_version.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :design_at_version, class: DesignManagement::DesignAtVersion do
+    skip_create # This is not an Active::Record model.
+
+    design { nil }
+
+    version { nil }
+
+    transient do
+      issue { design&.issue || version&.issue || create(:issue) }
+    end
+
+    initialize_with do
+      attrs = attributes.dup
+      attrs[:design] ||= create(:design, issue: issue)
+      attrs[:version] ||= create(:design_version, issue: issue)
+
+      new(attrs)
+    end
+  end
+end

ee/spec/factories/design_management/designs.rb

@@ -3,13 +3,25 @@
 FactoryBot.define do
   factory :design, class: DesignManagement::Design do
     issue { create(:issue) }
-    project { issue.project }
+    project { issue&.project || create(:project) }
     sequence(:filename) { |n| "homescreen-#{n}.jpg" }
 
     transient do
       author { issue.author }
     end
 
+    trait :importing do
+      issue { nil }
+
+      importing { true }
+      imported { false }
+    end
+
+    trait :imported do
+      importing { false }
+      imported { true }
+    end
+
     create_versions = ->(design, evaluator, commit_version) do
       unless evaluator.versions_count.zero?
         project = design.project
@@ -37,6 +49,8 @@ FactoryBot.define do
         # and maybe a deletion
         run_action[DesignManagement::DesignAction.new(design, :delete)] if evaluator.deleted
       end
+
+      design.clear_version_cache
     end
 
     trait :with_lfs_file do

ee/spec/factories/design_management/versions.rb

@@ -4,7 +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) }
+    author { issue&.author || create(:user) }
 
     transient do
       designs_count { 1 }
@@ -18,6 +18,19 @@ FactoryBot.define do
       designs_count { 0 }
     end
 
+    trait :importing do
+      issue { nil }
+
+      designs_count { 0 }
+      importing { true }
+      imported { false }
+    end
+
+    trait :imported do
+      importing { false }
+      imported { true }
+    end
+
     after(:build) do |version, evaluator|
       # By default all designs are created_designs, so just add them.
       specific_designs = [].concat(

ee/spec/factories/notes.rb

@@ -8,7 +8,7 @@ FactoryBot.modify do
     end
 
     trait :on_design do
-      noteable { create(:design, :with_file, project: project) }
+      noteable { create(:design, :with_file, issue: create(:issue, project: project)) }
     end
 
     trait :with_review do

ee/spec/finders/design_management/versions_finder_spec.rb

@@ -79,6 +79,20 @@ describe DesignManagement::VersionsFinder do
             it { is_expected.to contain_exactly(version_1, version_2) }
           end
         end
+
+        describe 'returning versions by SHA' do
+          context 'when argument is the first version' do
+            let(:params) { { sha: version_1.sha } }
+
+            it { is_expected.to contain_exactly(version_1) }
+          end
+
+          context 'when argument is the second version' do
+            let(:params) { { sha: version_2.sha } }
+
+            it { is_expected.to contain_exactly(version_2) }
+          end
+        end
       end
     end
   end

ee/spec/models/design_management/design_at_version_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DesignManagement::DesignAtVersion do
+  include DesignManagementTestHelpers
+
+  set(:issue) { create(:issue) }
+  set(:issue_b) { create(:issue) }
+  set(:design) { create(:design, issue: issue) }
+  set(:version) { create(:design_version, designs: [design]) }
+
+  describe '#id' do
+    subject { described_class.new(design: design, version: version) }
+
+    it 'combines design.id and version.id' do
+      expect(subject.id).to include(design.id.to_s, version.id.to_s)
+    end
+  end
+
+  describe 'status methods' do
+    let!(:design_a) { create(:design, issue: issue) }
+    let!(:design_b) { create(:design, issue: issue) }
+
+    let!(:version_a) do
+      create(:design_version, designs: [design_a])
+    end
+    let!(:version_b) do
+      create(:design_version, designs: [design_b])
+    end
+    let!(:version_mod) do
+      create(:design_version, modified_designs: [design_a, design_b])
+    end
+    let!(:version_c) do
+      create(:design_version, deleted_designs: [design_a])
+    end
+    let!(:version_d) do
+      create(:design_version, deleted_designs: [design_b])
+    end
+    let!(:version_e) do
+      create(:design_version, designs: [design_a])
+    end
+
+    describe 'a design before it has been created' do
+      subject { build(:design_at_version, design: design_b, version: version_a) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :not_created_yet' do
+        expect(subject).to have_attributes(status: :not_created_yet)
+      end
+    end
+
+    describe 'a design as of its creation' do
+      subject { build(:design_at_version, design: design_a, version: version_a) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :current' do
+        expect(subject).to have_attributes(status: :current)
+      end
+    end
+
+    describe 'a design after it has been created, but before deletion' do
+      subject { build(:design_at_version, design: design_b, version: version_c) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :current' do
+        expect(subject).to have_attributes(status: :current)
+      end
+    end
+
+    describe 'a design as of its modification' do
+      subject { build(:design_at_version, design: design_a, version: version_mod) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :current' do
+        expect(subject).to have_attributes(status: :current)
+      end
+    end
+
+    describe 'a design as of its deletion' do
+      subject { build(:design_at_version, design: design_a, version: version_c) }
+
+      it 'is deleted' do
+        expect(subject).to be_deleted
+      end
+
+      it 'has the status :deleted' do
+        expect(subject).to have_attributes(status: :deleted)
+      end
+    end
+
+    describe 'a design after its deletion' do
+      subject { build(:design_at_version, design: design_b, version: version_e) }
+
+      it 'is deleted' do
+        expect(subject).to be_deleted
+      end
+
+      it 'has the status :deleted' do
+        expect(subject).to have_attributes(status: :deleted)
+      end
+    end
+
+    describe 'a design on its recreation' do
+      subject { build(:design_at_version, design: design_a, version: version_e) }
+
+      it 'is not deleted' do
+        expect(subject).not_to be_deleted
+      end
+
+      it 'has the status :current' do
+        expect(subject).to have_attributes(status: :current)
+      end
+    end
+  end
+
+  describe 'validations' do
+    subject(:design_at_version) { build(:design_at_version) }
+
+    it { is_expected.to be_valid }
+
+    describe 'a design-at-version without a design' do
+      subject { described_class.new(design: nil, version: build(:design_version)) }
+
+      it { is_expected.to be_invalid }
+
+      it 'mentions the design in the errors' do
+        subject.valid?
+
+        expect(subject.errors[:design]).to be_present
+      end
+    end
+
+    describe 'a design-at-version without a version' do
+      subject { described_class.new(design: build(:design), version: nil) }
+
+      it { is_expected.to be_invalid }
+
+      it 'mentions the version in the errors' do
+        subject.valid?
+
+        expect(subject.errors[:version]).to be_present
+      end
+    end
+
+    describe 'design_and_version_belong_to_the_same_issue' do
+      context 'both design and version are supplied' do
+        subject(:design_at_version) { build(:design_at_version, design: design, version: version) }
+
+        context 'the design belongs to the same issue as the version' do
+          it { is_expected.to be_valid }
+        end
+
+        context 'the design does not belong to the same issue as the version' do
+          let(:design) { create(:design) }
+          let(:version) { create(:design_version) }
+
+          it { is_expected.to be_invalid }
+        end
+      end
+
+      context 'the factory is just supplied with a design' do
+        let(:design) { create(:design) }
+
+        subject(:design_at_version) { build(:design_at_version, design: design) }
+
+        it { is_expected.to be_valid }
+      end
+
+      context 'the factory is just supplied with a version' do
+        let(:version) { create(:design_version) }
+
+        subject(:design_at_version) { build(:design_at_version, version: version) }
+
+        it { is_expected.to be_valid }
+      end
+    end
+
+    describe 'design_and_version_have_issue_id' do
+      subject(:design_at_version) { build(:design_at_version, design: design, version: version) }
+
+      context 'the design has no issue_id, because it is being imported' do
+        let(:design) { create(:design, :importing) }
+
+        it { is_expected.to be_invalid }
+      end
+
+      context 'the version has no issue_id, because it is being imported' do
+        let(:version) { create(:design_version, :importing) }
+
+        it { is_expected.to be_invalid }
+      end
+
+      context 'both the design and the version are being imported' do
+        let(:version) { create(:design_version, :importing) }
+        let(:design) { create(:design, :importing) }
+
+        it { is_expected.to be_invalid }
+      end
+    end
+  end
+
+  def id_of(design, version)
+    build(:design_at_version, design: design, version: version).id
+  end
+
+  describe '.instantiate' do
+    context 'when attrs are valid' do
+      subject do
+        described_class.instantiate(design: design, version: version)
+      end
+
+      it { is_expected.to be_a(described_class).and(be_valid) }
+    end
+
+    context 'when attrs are invalid' do
+      subject do
+        described_class.instantiate(
+          design: create(:design),
+          version: create(:design_version)
+        )
+      end
+
+      it 'raises a validation error' do
+        expect { subject }.to raise_error(ActiveModel::ValidationError)
+      end
+    end
+  end
+
+  describe '.lazy_find' do
+    let!(:version_a) do
+      create(:design_version, designs: create_list(:design, 3, issue: issue))
+    end
+    let!(:version_b) do
+      create(:design_version, designs: create_list(:design, 1, issue: issue))
+    end
+    let!(:version_c) do
+      create(:design_version, designs: create_list(:design, 1, issue: issue_b))
+    end
+
+    let(:id_a)   { id_of(version_a.designs.first,  version_a) }
+    let(:id_b)   { id_of(version_a.designs.second, version_a) }
+    let(:id_c)   { id_of(version_a.designs.last,   version_a) }
+    let(:id_d)   { id_of(version_b.designs.first,  version_b) }
+    let(:id_e)   { id_of(version_c.designs.first,  version_c) }
+    let(:bad_id) { id_of(version_c.designs.first,  version_a) }
+
+    def find(the_id)
+      described_class.lazy_find(the_id)
+    end
+
+    let(:db_calls) { 2 }
+
+    it 'issues fewer queries than the naive approach would' do
+      expect do
+        dav_a = find(id_a)
+        dav_b = find(id_b)
+        dav_c = find(id_c)
+        dav_d = find(id_d)
+        dav_e = find(id_e)
+        should_not_exist = find(bad_id)
+
+        expect(dav_a.version).to eq(version_a)
+        expect(dav_b.version).to eq(version_a)
+        expect(dav_c.version).to eq(version_a)
+        expect(dav_d.version).to eq(version_b)
+        expect(dav_e.version).to eq(version_c)
+        expect(should_not_exist).not_to be_present
+
+        expect(version_a.designs).to include(dav_a.design, dav_b.design, dav_c.design)
+        expect(version_b.designs).to include(dav_d.design)
+        expect(version_c.designs).to include(dav_e.design)
+      end.not_to exceed_query_limit(db_calls)
+    end
+  end
+
+  describe '.find' do
+    let(:results) { described_class.find(ids) }
+
+    # 2 versions, with 5 total designs on issue A, so 2*5 = 10
+    let!(:version_a) do
+      create(:design_version, designs: create_list(:design, 3, issue: issue))
+    end
+    let!(:version_b) do
+      create(:design_version, designs: create_list(:design, 2, issue: issue))
+    end
+    # 1 version, with 3 designs on issue B, so 1*3 = 3
+    let!(:version_c) do
+      create(:design_version, designs: create_list(:design, 3, issue: issue_b))
+    end
+
+    context 'invalid ids' do
+      let(:ids) do
+        version_b.designs.map { |d| id_of(d, version_c) }
+      end
+
+      describe '#count' do
+        it 'counts 0 records' do
+          expect(results.count).to eq(0)
+        end
+      end
+
+      describe '#empty?' do
+        it 'is empty' do
+          expect(results).to be_empty
+        end
+      end
+
+      describe '#to_a' do
+        it 'finds no records' do
+          expect(results.to_a).to eq([])
+        end
+      end
+    end
+
+    context 'valid ids' do
+      let(:red_herrings) { issue_b.designs.sample(2).map { |d| id_of(d, version_a) } }
+
+      let(:ids) do
+        a_ids = issue.designs.sample(2).map { |d| id_of(d, version_a) }
+        b_ids = issue.designs.sample(2).map { |d| id_of(d, version_b) }
+        c_ids = issue_b.designs.sample(2).map { |d| id_of(d, version_c) }
+
+        a_ids + b_ids + c_ids + red_herrings
+      end
+
+      before do
+        ids.size # force IDs
+      end
+
+      describe '#count' do
+        it 'counts 2 records' do
+          expect(results.count).to eq(6)
+        end
+
+        it 'issues at most two queries' do
+          expect { results.count }.not_to exceed_query_limit(2)
+        end
+      end
+
+      describe '#to_a' do
+        it 'finds 6 records' do
+          expect(results.size).to eq(6)
+          expect(results).to all(be_a(described_class))
+        end
+
+        it 'only returns records with matching IDs' do
+          expect(results.map(&:id)).to match_array(ids - red_herrings)
+        end
+
+        it 'only returns valid records' do
+          expect(results).to all(be_valid)
+        end
+
+        it 'issues at most two queries' do
+          expect { results.to_a }.not_to exceed_query_limit(2)
+        end
+      end
+    end
+  end
+end

ee/spec/models/design_management/version_spec.rb

@@ -2,6 +2,8 @@
 require 'spec_helper'
 
 describe DesignManagement::Version do
+  let_it_be(:issue) { create(:issue) }
+
   describe 'relations' do
     it { is_expected.to have_many(:actions) }
     it { is_expected.to have_many(:designs).through(:actions) }
@@ -60,6 +62,14 @@ describe DesignManagement::Version do
         end
       end
     end
+
+    describe '.by_sha' do
+      it 'can find versions by sha' do
+        [version_1, version_2].each do |version|
+          expect(described_class.by_sha(version.sha)).to contain_exactly(version)
+        end
+      end
+    end
   end
 
   describe ".create_for_designs" do
@@ -74,7 +84,6 @@ describe DesignManagement::Version do
     end
 
     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] }
@@ -283,4 +292,51 @@ describe DesignManagement::Version do
       expect(version.author).to eq(commit_user)
     end
   end
+
+  describe '#diff_refs' do
+    let(:project) { issue.project }
+
+    before do
+      expect(project.design_repository).to receive(:commit)
+        .once
+        .with(sha)
+        .and_return(commit)
+    end
+
+    subject { create(:design_version, issue: issue, sha: sha) }
+
+    context 'there is a commit in the repo by the SHA' do
+      let(:commit) { build(:commit) }
+      let(:sha) { commit.id }
+
+      it { is_expected.to have_attributes(diff_refs: commit.diff_refs) }
+
+      it 'memoizes calls to #diff_refs' do
+        expect(subject.diff_refs).to eq(subject.diff_refs)
+      end
+    end
+
+    context 'there is no commit in the repo by the SHA' do
+      let(:commit) { nil }
+      let(:sha) { Digest::SHA1.hexdigest("points to nothing") }
+
+      it { is_expected.to have_attributes(diff_refs: be_nil) }
+    end
+  end
+
+  describe '#reset' do
+    subject { create(:design_version, issue: issue) }
+
+    it 'removes memoized values' do
+      expect(subject).to receive(:commit).twice.and_return(nil)
+
+      subject.diff_refs
+      subject.diff_refs
+
+      subject.reset
+
+      subject.diff_refs
+      subject.diff_refs
+    end
+  end
 end