Merge branch 'bvl-design-diff-notes' into 'master'

Modify `DiffNote` to reuse it for Designs

See merge request gitlab-org/gitlab-ee!13703

app/models/concerns/noteable.rb

@@ -3,14 +3,16 @@
 module Noteable
   extend ActiveSupport::Concern
 
-  # `Noteable` class names that support resolvable notes.
-  RESOLVABLE_TYPES = %w(MergeRequest).freeze
-
   class_methods do
     # `Noteable` class names that support replying to individual notes.
     def replyable_types
       %w(Issue MergeRequest)
     end
+
+    # `Noteable` class names that support resolvable notes.
+    def resolvable_types
+      %w(MergeRequest)
+    end
   end
 
   # The timestamp of the note (e.g. the :created_at or :updated_at attribute if provided via
@@ -36,7 +38,7 @@ module Noteable
   end
 
   def supports_resolvable_notes?
-    RESOLVABLE_TYPES.include?(base_class_name)
+    self.class.resolvable_types.include?(base_class_name)
   end
 
   def supports_discussions?
@@ -132,5 +134,7 @@ module Noteable
   end
 end
 
+Noteable.extend(Noteable::ClassMethods)
+
 Noteable::ClassMethods.prepend(EE::Noteable::ClassMethods) # rubocop: disable Cop/InjectEnterpriseEditionModule
 Noteable.prepend(EE::Noteable)

app/models/concerns/resolvable_note.rb

@@ -12,7 +12,7 @@ module ResolvableNote
     validates :resolved_by, presence: true, if: :resolved?
 
     # Keep this scope in sync with `#potentially_resolvable?`
-    scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: Noteable::RESOLVABLE_TYPES) }
+    scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: Noteable.resolvable_types) }
     # Keep this scope in sync with `#resolvable?`
     scope :resolvable, -> { potentially_resolvable.user }
 

app/models/diff_note.rb

@@ -15,7 +15,9 @@ class DiffNote < Note
   validates :original_position, presence: true
   validates :position, presence: true
   validates :line_code, presence: true, line_code: true, if: :on_text?
-  validates :noteable_type, inclusion: { in: noteable_types }
+  # We need to evaluate the `noteable` types when running the validation since
+  # EE might have added a type when the module was prepended
+  validates :noteable_type, inclusion: { in: -> (_note) { noteable_types } }
   validate :positions_complete
   validate :verify_supported
   validate :diff_refs_match_commit, if: :for_commit?
@@ -44,7 +46,7 @@ class DiffNote < Note
   # Returns the diff file from `position`
   def latest_diff_file
     strong_memoize(:latest_diff_file) do
-      position.diff_file(project.repository)
+      position.diff_file(repository)
     end
   end
 
@@ -111,7 +113,7 @@ class DiffNote < Note
       if note_diff_file
         diff = Gitlab::Git::Diff.new(note_diff_file.to_hash)
         Gitlab::Diff::File.new(diff,
-                               repository: project.repository,
+                               repository: repository,
                                diff_refs: original_position.diff_refs)
       elsif created_at_diff?(noteable.diff_refs)
         # We're able to use the already persisted diffs (Postgres) if we're
@@ -122,7 +124,7 @@ class DiffNote < Note
         # `Diff::FileCollection::MergeRequestDiff`.
         noteable.diffs(original_position.diff_options).diff_files.first
       else
-        original_position.diff_file(self.project.repository)
+        original_position.diff_file(repository)
       end
 
     # Since persisted diff files already have its content "unfolded"
@@ -137,7 +139,7 @@ class DiffNote < Note
   end
 
   def set_line_code
-    self.line_code = self.position.line_code(self.project.repository)
+    self.line_code = self.position.line_code(repository)
   end
 
   def verify_supported
@@ -171,6 +173,12 @@ class DiffNote < Note
       shas << self.position.head_sha
     end
 
-    project.repository.keep_around(*shas)
+    repository.keep_around(*shas)
+  end
+
+  def repository
+    noteable.respond_to?(:repository) ? noteable.repository : project.repository
   end
 end
+
+DiffNote.prepend(::EE::DiffNote)

ee/app/models/concerns/ee/noteable.rb

@@ -11,6 +11,10 @@ module EE
       def replyable_types
         super + %w(Epic)
       end
+
+      def resolvable_types
+        super + %w(DesignManagement::Design)
+      end
     end
 
     override :note_etag_key

ee/app/models/design_management/design.rb

@@ -2,6 +2,7 @@
 
 module DesignManagement
   class Design < ApplicationRecord
+    include Noteable
     include Gitlab::FileTypeDetection
 
     belongs_to :project, inverse_of: :designs
@@ -9,6 +10,9 @@ module DesignManagement
 
     has_many :design_versions
     has_many :versions, through: :design_versions, class_name: 'DesignManagement::Version', inverse_of: :designs
+    # This is a polymorphic association, so we can't count on FK's to delete the
+    # data
+    has_many :notes, as: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
     validates :project, :issue, :filename, presence: true
     validates :filename, uniqueness: { scope: :issue_id }
@@ -22,8 +26,22 @@ module DesignManagement
       @full_path ||= File.join(DesignManagement.designs_directory, "issue-#{issue.iid}", filename)
     end
 
+    def diff_refs
+      return if new_design?
+
+      @diff_refs ||= repository.commit(head_version.sha).diff_refs
+    end
+
+    def repository
+      project.design_repository
+    end
+
     private
 
+    def head_version
+      @head_sha ||= versions.ordered.first
+    end
+
     def validate_file_is_image
       unless image?
         message = _("Only these extensions are supported: %{extension_list}") % {

ee/app/models/design_management/version.rb

@@ -11,10 +11,6 @@ module DesignManagement
              source: :design,
              inverse_of: :versions
 
-    # This is a polymorphic association, so we can't count on FK's to delete the
-    # data
-    has_many :notes, as: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
-
     validates :sha, presence: true
     validates :sha, uniqueness: { case_sensitive: false }
 

ee/app/models/ee/diff_note.rb

+# frozen_string_literal: true
+
+module EE
+  module DiffNote
+    extend ::Gitlab::Utils::Override
+    extend ActiveSupport::Concern
+
+    class_methods do
+      def noteable_types
+        super + %w(DesignManagement::Design)
+      end
+    end
+
+    override :supported?
+    def supported?
+      for_design? || super
+    end
+
+    # diffs are currently not suported for designs
+    # this needs to be decoupled from the `Project#repository`
+    override :latest_diff_file
+    def latest_diff_file
+      return super unless for_design?
+    end
+
+    override :diff_file
+    def diff_file
+      return super unless for_design?
+    end
+  end
+end

ee/app/models/ee/note.rb

@@ -47,6 +47,10 @@ module EE
       for_epic? || super
     end
 
+    def for_design?
+      noteable.is_a?(DesignManagement::Design)
+    end
+
     override :parent
     def parent
       for_epic? ? noteable.group : super

ee/spec/factories/notes.rb

@@ -15,4 +15,25 @@ end
 
 FactoryBot.define do
   factory :note_on_epic, parent: :note, traits: [:on_epic]
+
+  factory :diff_note_on_design, class: DiffNote do
+    association :project
+    note { generate(:title) }
+    author { project&.creator || create(:user) }
+
+    noteable { create(:design, :with_file, project: project) }
+
+    position do
+      Gitlab::Diff::Position.new(
+        old_path: noteable.full_path,
+        new_path: noteable.full_path,
+        width: 10,
+        height: 10,
+        x: 1,
+        y: 1,
+        position_type: "image",
+        diff_refs: noteable.diff_refs
+      )
+    end
+  end
 end

ee/spec/models/concerns/ee/noteable_spec.rb

@@ -3,13 +3,17 @@
 require 'rails_helper'
 
 describe EE::Noteable do
+  subject(:klazz) { Class.new { include Noteable } }
+
   describe '.replyable_types' do
     it 'adds Epic to replyable_types after being included' do
-      class SomeClass
-        include Noteable
-      end
+      expect(klazz.replyable_types).to include("Epic")
+    end
+  end
 
-      expect(SomeClass.replyable_types).to include("Epic")
+  describe '.resolvable_types' do
+    it 'includes design management' do
+      expect(klazz.resolvable_types).to include('DesignManagement::Design')
     end
   end
 end

ee/spec/models/design_management/design_spec.rb

@@ -8,6 +8,7 @@ describe DesignManagement::Design do
     it { is_expected.to belong_to(:issue) }
     it { is_expected.to have_many(:design_versions) }
     it { is_expected.to have_many(:versions) }
+    it { is_expected.to have_many(:notes).dependent(:delete_all) }
   end
 
   describe 'validations' do
@@ -61,4 +62,28 @@ describe DesignManagement::Design do
       expect(design.full_path).to eq(expected_path)
     end
   end
+
+  describe '#diff_refs' do
+    it "builds diff refs based on the first commit and it's for the design" do
+      design = create(:design, :with_file, versions_count: 3)
+
+      expect(design.diff_refs.base_sha).to eq(design.versions.ordered.second.sha)
+      expect(design.diff_refs.head_sha).to eq(design.versions.ordered.first.sha)
+    end
+
+    it 'builds diff refs based on the empty tree if there was only one version' do
+      design = create(:design, :with_file, versions_count: 1)
+
+      expect(design.diff_refs.base_sha).to eq(Gitlab::Git::BLANK_SHA)
+      expect(design.diff_refs.head_sha).to eq(design.diff_refs.head_sha)
+    end
+  end
+
+  describe '#repository' do
+    it 'is a design repository' do
+      design = build(:design)
+
+      expect(design.repository).to be_a(DesignManagement::Repository)
+    end
+  end
 end

ee/spec/models/design_management/version_spec.rb

@@ -22,8 +22,6 @@ describe DesignManagement::Version do
       expect { versions.each { |v| design.versions << v } }
         .not_to raise_error
     end
-
-    it { is_expected.to have_many(:notes).dependent(:delete_all) }
   end
 
   describe 'validations' do

ee/spec/models/ee/diff_note_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe DiffNote do
+  describe 'Validations' do
+    it 'allows diffnotes on designs' do
+      diff_note = build(:diff_note_on_design)
+
+      expect(diff_note).to be_valid
+    end
+  end
+
+  context 'diff files' do
+    let(:design) { create(:design, :with_file, versions_count: 2) }
+    let(:diff_note) { create(:diff_note_on_design, noteable: design, project: design.project) }
+
+    describe '#latest_diff_file' do
+      it 'does not return a diff file' do
+        expect(diff_note.latest_diff_file).to be_nil
+      end
+    end
+
+    describe '#diff_file' do
+      it 'does not return a diff file' do
+        expect(diff_note.diff_file).to be_nil
+      end
+    end
+  end
+end

ee/spec/models/ee/note_spec.rb

@@ -38,4 +38,12 @@ describe Note do
       expect(note.parent).to eq(group)
     end
   end
+
+  describe '#for_design' do
+    it 'is true when the noteable is a design' do
+      note = build(:note, noteable: build(:design))
+
+      expect(note).to be_for_design
+    end
+  end
 end

ee/spec/services/ee/notes/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Notes::CreateService do
+  context 'notes for designs' do
+    set(:design) { create(:design, :with_file) }
+    set(:project) { design.project }
+    set(:user) { project.owner }
+
+    subject(:service) { described_class.new(project, user, opts) }
+
+    describe "#execute" do
+      let(:opts) do
+        {
+          type: 'DiffNote',
+          noteable: design,
+          note: "A message",
+          position: {
+            old_path: design.full_path,
+            new_path: design.full_path,
+            position_type: 'image',
+            width: '100',
+            height: '100',
+            x: '50',
+            y: '50',
+            base_sha: design.diff_refs.base_sha,
+            start_sha: design.diff_refs.base_sha,
+            head_sha: design.diff_refs.head_sha
+          }
+        }
+      end
+
+      it 'can create diff notes for designs' do
+        note = service.execute
+
+        expect(note).to be_a(DiffNote)
+        expect(note).to be_persisted
+        expect(note.noteable).to eq(design)
+      end
+
+      it 'correctly builds the position of the note' do
+        note = service.execute
+
+        expect(note.position.new_path).to eq(design.full_path)
+        expect(note.position.old_path).to eq(design.full_path)
+        expect(note.position.diff_refs).to eq(design.diff_refs)
+      end
+    end
+  end
+end

lib/api/discussions.rb

@@ -206,7 +206,7 @@ module API
           delete_note(noteable, params[:note_id])
         end
 
-        if Noteable::RESOLVABLE_TYPES.include?(noteable_type.to_s)
+        if Noteable.resolvable_types.include?(noteable_type.to_s)
           desc "Resolve/unresolve an existing #{noteable_type.to_s.downcase} discussion" do
             success Entities::Discussion
           end

spec/models/concerns/noteable_spec.rb

@@ -260,4 +260,16 @@ describe Noteable do
       end
     end
   end
+
+  describe '.replyable_types' do
+    it 'exposes the replyable types' do
+      expect(described_class.replyable_types).to include('Issue', 'MergeRequest')
+    end
+  end
+
+  describe '.resolvable_types' do
+    it 'exposes the replyable types' do
+      expect(described_class.resolvable_types).to include('MergeRequest')
+    end
+  end
 end