Merge branch 'improve-diff-refs-and-factories' into 'master'

Improve diff refs and factories

Closes #42741

See merge request gitlab-org/gitlab!21337

app/models/diff_note.rb

@@ -161,7 +161,7 @@ class DiffNote < Note
   def positions_complete
     return if self.original_position.complete? && self.position.complete?
 
-    errors.add(:position, "is invalid")
+    errors.add(:position, "is incomplete")
   end
 
   def keep_around_commits

ee/spec/factories/notes.rb

@@ -21,17 +21,6 @@ FactoryBot.define do
   factory :note_on_epic, parent: :note, traits: [:on_epic]
 
   factory :diff_note_on_design, parent: :note, traits: [:on_design], class: 'DiffNote' do
-    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
+    position { build(:image_diff_position, file: noteable.full_path, diff_refs: noteable.diff_refs) }
   end
 end

lib/gitlab/diff/diff_refs.rb

@@ -37,7 +37,7 @@ module Gitlab
       # We have `base_sha` directly available on `DiffRefs` because it's faster#
       # than having to look it up in the repo every time.
       def complete?
-        start_sha && head_sha
+        start_sha.present? && head_sha.present?
       end
 
       def compare_in(project)

lib/gitlab/diff/formatters/image_formatter.rb

@@ -23,7 +23,7 @@ module Gitlab
         end
 
         def complete?
-          x && y && width && height
+          [x, y, width, height].all?(&:present?)
         end
 
         def to_h

lib/gitlab/diff/formatters/text_formatter.rb

@@ -19,7 +19,7 @@ module Gitlab
         end
 
         def complete?
-          old_line || new_line
+          old_line.present? || new_line.present?
         end
 
         def to_h

spec/factories/diff_position.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :diff_position, class: 'Gitlab::Diff::Position' do
+    skip_create # non-model factories (i.e. without #save)
+
+    transient do
+      file { 'path/to/file' }
+
+      # Allow diff to be passed as a single object.
+      diff_refs do
+        ::Gitlab::Diff::DiffRefs.new(
+          base_sha: Digest::SHA1.hexdigest(SecureRandom.hex),
+          head_sha: Digest::SHA1.hexdigest(SecureRandom.hex),
+          start_sha: Digest::SHA1.hexdigest(SecureRandom.hex)
+        )
+      end
+    end
+
+    old_path { file }
+    new_path { file }
+
+    base_sha  { diff_refs&.base_sha }
+    head_sha  { diff_refs&.head_sha }
+    start_sha { diff_refs&.start_sha }
+
+    initialize_with { new(attributes) }
+
+    trait :moved do
+      new_path { 'path/to/new.file' }
+    end
+
+    factory :text_diff_position do
+      position_type { 'text' }
+      old_line { 10 }
+      new_line { 10 }
+
+      trait :added do
+        old_line { nil }
+      end
+    end
+
+    factory :image_diff_position do
+      position_type { 'image' }
+      x { 1 }
+      y { 1 }
+      width { 10 }
+      height { 10 }
+    end
+  end
+end

spec/factories/notes.rb

@@ -58,24 +58,20 @@ FactoryBot.define do
       end
 
       position do
-        Gitlab::Diff::Position.new(
-          old_path: "files/ruby/popen.rb",
-          new_path: "files/ruby/popen.rb",
-          old_line: nil,
-          new_line: line_number,
-          diff_refs: diff_refs
-        )
+        build(:text_diff_position,
+              file: "files/ruby/popen.rb",
+              old_line: nil,
+              new_line: line_number,
+              diff_refs: diff_refs)
       end
 
       trait :folded_position do
         position do
-          Gitlab::Diff::Position.new(
-            old_path: "files/ruby/popen.rb",
-            new_path: "files/ruby/popen.rb",
-            old_line: 1,
-            new_line: 1,
-            diff_refs: diff_refs
-          )
+          build(:text_diff_position,
+                file: "files/ruby/popen.rb",
+                old_line: 1,
+                new_line: 1,
+                diff_refs: diff_refs)
         end
       end
 
@@ -86,16 +82,9 @@ FactoryBot.define do
 
       factory :image_diff_note_on_merge_request do
         position do
-          Gitlab::Diff::Position.new(
-            old_path: "files/images/any_image.png",
-            new_path: "files/images/any_image.png",
-            width: 10,
-            height: 10,
-            x: 1,
-            y: 1,
-            diff_refs: diff_refs,
-            position_type: "image"
-          )
+          build(:image_diff_position,
+                file: "files/images/any_image.png",
+                diff_refs: diff_refs)
         end
       end
     end
@@ -109,9 +98,8 @@ FactoryBot.define do
       end
 
       position do
-        Gitlab::Diff::Position.new(
-          old_path: "files/ruby/popen.rb",
-          new_path: "files/ruby/popen.rb",
+        build(:text_diff_position,
+          file: "files/ruby/popen.rb",
           old_line: nil,
           new_line: line_number,
           diff_refs: diff_refs

spec/features/merge_request/user_creates_image_diff_notes_spec.rb

@@ -48,29 +48,11 @@ describe 'Merge request > User creates image diff notes', :js do
       let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') }
 
       let(:note1_position) do
-        Gitlab::Diff::Position.new(
-          old_path: path,
-          new_path: path,
-          width: 100,
-          height: 100,
-          x: 10,
-          y: 10,
-          position_type: "image",
-          diff_refs: commit.diff_refs
-        )
+        build(:image_diff_position, file: path, diff_refs: commit.diff_refs)
       end
 
       let(:note2_position) do
-        Gitlab::Diff::Position.new(
-          old_path: path,
-          new_path: path,
-          width: 100,
-          height: 100,
-          x: 20,
-          y: 20,
-          position_type: "image",
-          diff_refs: commit.diff_refs
-        )
+        build(:image_diff_position, file: path, diff_refs: commit.diff_refs)
       end
 
       let!(:note1) { create(:diff_note_on_commit, commit_id: commit.id, project: project, position: note1_position, note: 'my note 1') }
@@ -93,16 +75,7 @@ describe 'Merge request > User creates image diff notes', :js do
   %w(inline parallel).each do |view|
     context "#{view} view" do
       let(:position) do
-        Gitlab::Diff::Position.new(
-          old_path: path,
-          new_path: path,
-          width: 100,
-          height: 100,
-          x: 1,
-          y: 1,
-          position_type: "image",
-          diff_refs: merge_request.diff_refs
-        )
+        build(:image_diff_position, file: path, diff_refs: merge_request.diff_refs)
       end
 
       let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) }
@@ -167,16 +140,7 @@ describe 'Merge request > User creates image diff notes', :js do
     let(:path)          { "files/images/ee_repo_logo.png" }
 
     let(:position) do
-      Gitlab::Diff::Position.new(
-        old_path: path,
-        new_path: path,
-        width: 100,
-        height: 100,
-        x: 50,
-        y: 50,
-        position_type: "image",
-        diff_refs: merge_request.diff_refs
-      )
+      build(:image_diff_position, file: path, diff_refs: merge_request.diff_refs)
     end
 
     before do

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -10,13 +10,9 @@ describe 'Merge request > User resolves diff notes and threads', :js do
   let!(:note)         { create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "| Markdown | Table |\n|-------|---------|\n| first | second |") }
   let(:path)          { "files/ruby/popen.rb" }
   let(:position) do
-    Gitlab::Diff::Position.new(
-      old_path: path,
-      new_path: path,
-      old_line: nil,
-      new_line: 9,
-      diff_refs: merge_request.diff_refs
-    )
+    build(:text_diff_position,
+          file: path, old_line: nil, new_line: 9,
+          diff_refs: merge_request.diff_refs)
   end
 
   before do

spec/features/merge_request/user_resolves_outdated_diff_discussions_spec.rb

@@ -13,20 +13,16 @@ describe 'Merge request > User resolves outdated diff discussions', :js do
   let(:current_diff_refs) { merge_request.diff_refs }
 
   let(:outdated_position) do
-    Gitlab::Diff::Position.new(
-      old_path: 'files/csv/Book1.csv',
-      new_path: 'files/csv/Book1.csv',
-      old_line: nil,
+    build(:text_diff_position, :added,
+      file: 'files/csv/Book1.csv',
       new_line: 9,
       diff_refs: outdated_diff_refs
     )
   end
 
   let(:current_position) do
-    Gitlab::Diff::Position.new(
-      old_path: 'files/csv/Book1.csv',
-      new_path: 'files/csv/Book1.csv',
-      old_line: nil,
+    build(:text_diff_position, :added,
+      file: 'files/csv/Book1.csv',
       new_line: 1,
       diff_refs: current_diff_refs
     )

spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb

@@ -10,10 +10,8 @@ describe 'Merge request > User sees avatars on diff notes', :js do
   let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: 'Bug NS-04') }
   let(:path)          { 'files/ruby/popen.rb' }
   let(:position) do
-    Gitlab::Diff::Position.new(
-      old_path: path,
-      new_path: path,
-      old_line: nil,
+    build(:text_diff_position, :added,
+      file: path,
       new_line: 9,
       diff_refs: merge_request.diff_refs
     )

spec/features/merge_request/user_sees_discussions_spec.rb

@@ -18,10 +18,8 @@ describe 'Merge request > User sees threads', :js do
     let!(:outdated_discussion) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position).to_discussion }
     let!(:active_discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
     let(:outdated_position) do
-      Gitlab::Diff::Position.new(
-        old_path: "files/ruby/popen.rb",
-        new_path: "files/ruby/popen.rb",
-        old_line: nil,
+      build(:text_diff_position, :added,
+        file: "files/ruby/popen.rb",
         new_line: 9,
         diff_refs: outdated_diff_refs
       )

spec/features/merge_request/user_sees_versions_spec.rb

@@ -86,10 +86,8 @@ describe 'Merge request > User sees versions', :js do
     it 'shows comments that were last relevant at that version' do
       expect(page).to have_content '5 files'
 
-      position = Gitlab::Diff::Position.new(
-        old_path: ".gitmodules",
-        new_path: ".gitmodules",
-        old_line: nil,
+      position = build(:text_diff_position, :added,
+        file: ".gitmodules",
         new_line: 4,
         diff_refs: merge_request_diff1.diff_refs
       )
@@ -136,9 +134,8 @@ describe 'Merge request > User sees versions', :js do
       expect(additions_content).to eq '15'
       expect(deletions_content).to eq '6'
 
-      position = Gitlab::Diff::Position.new(
-        old_path: ".gitmodules",
-        new_path: ".gitmodules",
+      position = build(:text_diff_position,
+        file: ".gitmodules",
         old_line: 4,
         new_line: 4,
         diff_refs: merge_request_diff3.compare_with(merge_request_diff1.head_commit_sha).diff_refs

spec/frontend/fixtures/merge_requests.rb

@@ -36,10 +36,8 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont
   end
   let(:path) { "files/ruby/popen.rb" }
   let(:position) do
-    Gitlab::Diff::Position.new(
-      old_path: path,
-      new_path: path,
-      old_line: nil,
+    build(:text_diff_position, :added,
+      file: path,
       new_line: 14,
       diff_refs: merge_request.diff_refs
     )
@@ -112,14 +110,8 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont
     let(:merge_request2) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, title: "Added images") }
     let(:image_path) { "files/images/ee_repo_logo.png" }
     let(:image_position) do
-      Gitlab::Diff::Position.new(
-        old_path: image_path,
-        new_path: image_path,
-        width: 100,
-        height: 100,
-        x: 1,
-        y: 1,
-        position_type: "image",
+      build(:image_diff_position,
+        file: image_path,
         diff_refs: merge_request2.diff_refs
       )
     end

spec/frontend/fixtures/merge_requests_diffs.rb

@@ -12,10 +12,8 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type
   let(:path) { "files/ruby/popen.rb" }
   let(:selected_commit) { merge_request.all_commits[0] }
   let(:position) do
-    Gitlab::Diff::Position.new(
-      old_path: path,
-      new_path: path,
-      old_line: nil,
+    build(:text_diff_position, :added,
+      file: path,
       new_line: 14,
       diff_refs: merge_request.diff_refs
     )

spec/helpers/notes_helper_spec.rb

@@ -62,10 +62,8 @@ describe NotesHelper do
 
         context 'when the discussion is on an older merge request version' do
           let(:position) do
-            Gitlab::Diff::Position.new(
-              old_path: ".gitmodules",
-              new_path: ".gitmodules",
-              old_line: nil,
+            build(:text_diff_position, :added,
+              file: ".gitmodules",
               new_line: 4,
               diff_refs: merge_request_diff1.diff_refs
             )
@@ -86,9 +84,8 @@ describe NotesHelper do
 
         context 'when the discussion is on a comparison between merge request versions' do
           let(:position) do
-            Gitlab::Diff::Position.new(
-              old_path: ".gitmodules",
-              new_path: ".gitmodules",
+            build(:text_diff_position,
+              file: ".gitmodules",
               old_line: 4,
               new_line: 4,
               diff_refs: merge_request_diff3.compare_with(merge_request_diff1.head_commit_sha).diff_refs

spec/lib/gitlab/diff/lines_unfolder_spec.rb

@@ -212,14 +212,7 @@ describe Gitlab::Diff::LinesUnfolder do
 
   context 'position requires a middle expansion and new match lines' do
     let(:position) do
-      Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 head_sha: "1487062132228de836236c522fe52fed4980a46c",
-                                 old_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 new_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 position_type:  "text",
-                                 old_line: 43,
-                                 new_line: 40)
+      build(:text_diff_position, old_line: 43, new_line: 40)
     end
 
     context 'blob lines' do
@@ -321,14 +314,7 @@ describe Gitlab::Diff::LinesUnfolder do
 
   context 'position requires a middle expansion and no top match line' do
     let(:position) do
-      Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 head_sha: "1487062132228de836236c522fe52fed4980a46c",
-                                 old_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 new_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 position_type:  "text",
-                                 old_line: 16,
-                                 new_line: 17)
+      build(:text_diff_position, old_line: 16, new_line: 17)
     end
 
     context 'blob lines' do
@@ -422,14 +408,7 @@ describe Gitlab::Diff::LinesUnfolder do
 
   context 'position requires a middle expansion and no bottom match line' do
     let(:position) do
-      Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 head_sha: "1487062132228de836236c522fe52fed4980a46c",
-                                 old_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 new_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 position_type:  "text",
-                                 old_line: 82,
-                                 new_line: 79)
+      build(:text_diff_position, old_line: 82, new_line: 79)
     end
 
     context 'blob lines' do
@@ -523,14 +502,7 @@ describe Gitlab::Diff::LinesUnfolder do
 
   context 'position requires a short top expansion' do
     let(:position) do
-      Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 head_sha: "1487062132228de836236c522fe52fed4980a46c",
-                                 old_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 new_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 position_type: "text",
-                                 old_line: 6,
-                                 new_line: 6)
+      build(:text_diff_position, old_line: 6, new_line: 6)
     end
 
     context 'blob lines' do
@@ -621,14 +593,7 @@ describe Gitlab::Diff::LinesUnfolder do
 
   context 'position sits between two match lines (no expasion needed)' do
     let(:position) do
-      Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 head_sha: "1487062132228de836236c522fe52fed4980a46c",
-                                 old_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 new_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 position_type: "text",
-                                 old_line: 64,
-                                 new_line: 61)
+      build(:text_diff_position, old_line: 64, new_line: 61)
     end
 
     context 'diff lines' do
@@ -640,14 +605,7 @@ describe Gitlab::Diff::LinesUnfolder do
 
   context 'position requires bottom expansion and new match lines' do
     let(:position) do
-      Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                 head_sha: "1487062132228de836236c522fe52fed4980a46c",
-                                 old_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 new_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                 position_type: "text",
-                                 old_line: 107,
-                                 new_line: 99)
+      build(:text_diff_position, old_line: 107, new_line: 99)
     end
 
     context 'blob lines' do
@@ -744,14 +702,7 @@ describe Gitlab::Diff::LinesUnfolder do
 
     context 'position requires bottom expansion and no new match line' do
       let(:position) do
-        Gitlab::Diff::Position.new(base_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                   start_sha: "1c59dfa64afbea8c721bb09a06a9d326c952ea19",
-                                   head_sha: "1487062132228de836236c522fe52fed4980a46c",
-                                   old_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                   new_path: "build-aux/flatpak/org.gnome.Nautilus.json",
-                                   position_type: "text",
-                                   old_line: 95,
-                                   new_line: 87)
+        build(:text_diff_position, old_line: 95, new_line: 87)
       end
 
       context 'blob lines' do
@@ -844,16 +795,7 @@ describe Gitlab::Diff::LinesUnfolder do
   end
 
   context 'positioned on an image' do
-    let(:position) do
-      Gitlab::Diff::Position.new(
-        base_sha: '1c59dfa64afbea8c721bb09a06a9d326c952ea19',
-        start_sha: '1c59dfa64afbea8c721bb09a06a9d326c952ea19',
-        head_sha: '1487062132228de836236c522fe52fed4980a46c',
-        old_path: 'image.jpg',
-        new_path: 'image.jpg',
-        position_type: 'image'
-      )
-    end
+    let(:position) { build(:image_diff_position) }
 
     before do
       allow(old_blob).to receive(:binary?).and_return(binary?)

spec/lib/gitlab/diff/position_collection_spec.rb

@@ -5,36 +5,17 @@ require 'spec_helper'
 describe Gitlab::Diff::PositionCollection do
   let(:merge_request) { build(:merge_request) }
 
-  def build_text_position(attrs = {})
-    attributes = {
-      old_path: "files/ruby/popen.rb",
-      new_path: "files/ruby/popen.rb",
-      old_line: nil,
-      new_line: 14,
-      diff_refs: merge_request.diff_refs
-    }.merge(attrs)
-
-    Gitlab::Diff::Position.new(attributes)
+  let(:text_position) do
+    build(:text_diff_position, :added, diff_refs: diff_refs)
   end
-
-  def build_image_position(attrs = {})
-    attributes = {
-      old_path: "files/images/any_image.png",
-      new_path: "files/images/any_image.png",
-      width: 10,
-      height: 10,
-      x: 1,
-      y: 1,
-      diff_refs: merge_request.diff_refs,
-      position_type: "image"
-    }.merge(attrs)
-
-    Gitlab::Diff::Position.new(attributes)
+  let(:folded_text_position) do
+    build(:text_diff_position, diff_refs: diff_refs, old_line: 1, new_line: 1)
+  end
+  let(:image_position) do
+    build(:image_diff_position, diff_refs: diff_refs)
   end
 
-  let(:text_position) { build_text_position }
-  let(:folded_text_position) { build_text_position(old_line: 1, new_line: 1) }
-  let(:image_position) { build_image_position }
+  let(:diff_refs) { merge_request.diff_refs }
   let(:invalid_position) { 'a position' }
   let(:head_sha) { merge_request.diff_head_sha }
 
@@ -71,7 +52,9 @@ describe Gitlab::Diff::PositionCollection do
   end
 
   describe '#concat' do
-    let(:new_text_position) { build_text_position(old_line: 1, new_line: 1) }
+    let(:new_text_position) do
+      build(:text_diff_position, diff_refs: diff_refs, old_line: 1, new_line: 1)
+    end
 
     it 'returns a Gitlab::Diff::Position' do
       expect(collection.concat([new_text_position])).to be_a(described_class)

spec/lib/gitlab/diff/position_spec.rb

@@ -35,6 +35,32 @@ describe Gitlab::Diff::Position do
     }
   end
 
+  describe 'factory' do
+    it 'produces a complete text position' do
+      position = build(:text_diff_position)
+
+      expect(position).to be_complete
+      expect(position).to have_attributes(position_type: 'text')
+    end
+
+    it 'produces a complete image position' do
+      position = build(:image_diff_position)
+
+      expect(position).to be_complete
+      expect(position).to have_attributes(position_type: 'image')
+    end
+
+    it 'allows the diff_refs to be passed as a single object' do
+      head_sha  = Digest::SHA1.hexdigest(SecureRandom.hex)
+      base_sha  = Digest::SHA1.hexdigest(SecureRandom.hex)
+      start_sha = Digest::SHA1.hexdigest(SecureRandom.hex)
+
+      refs = ::Gitlab::Diff::DiffRefs.new(base_sha: base_sha, start_sha: start_sha, head_sha: head_sha)
+
+      expect(build(:diff_position, diff_refs: refs).diff_refs).to eq(refs)
+    end
+  end
+
   describe "position for an added text file" do
     let(:commit) { project.commit("2ea1f3dec713d940208fb5ce4a38765ecb5d3f73") }