kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Merge branch 'dm-outdated-system-note' into 'master'

Browse source 
Add system note with link to diff comparison when MR discussion becomes outdated

Closes #30058

See merge request !11584
This commit is contained in:
Grzegorz Bizon 2017-05-24 19:00:26 +00:00
commit af50d334e7
33 changed files with 647 additions and 259 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
app/assets/stylesheets/framework/timeline.scss
View file

@ -5,7 +5,7 @@
.note-text {
p:last-child {
margin-bottom: 0;
margin-bottom: 0 !important;
}
}

4
app/assets/stylesheets/pages/note_form.scss
View file

@ -164,10 +164,6 @@
.discussion-body,
.diff-file {
.notes .note {
padding: 10px 15px;
}
.discussion-reply-holder {
background-color: $white-light;
padding: 10px 16px;

19
app/assets/stylesheets/pages/notes.scss
View file

@ -80,10 +80,6 @@ ul.notes {
&.timeline-entry {
padding: 14px 10px;
}
.system-note {
padding: 0;
}
}
&.is-editing {
@ -380,6 +376,10 @@ ul.notes {
padding-bottom: 5px;
}
.system-note .note-header-info {
padding-bottom: 0;
}
.note-headline-light {
display: inline;
@ -582,6 +582,17 @@ ul.notes {
}
}
.discussion-body,
.diff-file {
.notes .note {
padding: 10px 15px;
&.system-note {
padding: 0;
}
}
}
.diff-file {
.is-over {
.add-diff-note {

3
app/helpers/system_note_helper.rb
View file

@ -17,7 +17,8 @@ module SystemNoteHelper
'visible' => 'icon_eye',
'milestone' => 'icon_clock_o',
'discussion' => 'icon_comment_o',
'moved' => 'icon_arrow_circle_o_right'
'moved' => 'icon_arrow_circle_o_right',
'outdated' => 'icon_edit'
}.freeze
def icon_for_system_note(note)

8
app/models/concerns/discussion_on_diff.rb
View file

@ -47,4 +47,12 @@ module DiscussionOnDiff
prev_lines
end
def line_code_in_diffs(diff_refs)
if active?(diff_refs)
line_code
elsif diff_refs && created_at_diff?(diff_refs)
original_line_code
end
end
end

16
app/models/diff_discussion.rb
View file

@ -19,21 +19,9 @@ class DiffDiscussion < Discussion
def merge_request_version_params
return unless for_merge_request?
return {} if active?
if active?
{}
else
diff_refs = position.diff_refs
if diff = noteable.merge_request_diff_for(diff_refs)
{ diff_id: diff.id }
elsif diff = noteable.merge_request_diff_for(diff_refs.head_sha)
{
diff_id: diff.id,
start_sha: diff_refs.start_sha
}
end
end
noteable.version_params_for(position.diff_refs)
end
def reply_attributes

7
app/models/diff_note.rb
View file

@ -8,6 +8,7 @@ class DiffNote < Note
serialize :original_position, Gitlab::Diff::Position
serialize :position, Gitlab::Diff::Position
serialize :change_position, Gitlab::Diff::Position
validates :original_position, presence: true
validates :position, presence: true
@ -25,7 +26,7 @@ class DiffNote < Note
DiffDiscussion
end
%i(original_position position).each do |meth|
%i(original_position position change_position).each do |meth|
define_method "#{meth}=" do |new_position|
if new_position.is_a?(String)
new_position = JSON.parse(new_position) rescue nil
@ -36,6 +37,8 @@ class DiffNote < Note
new_position = Gitlab::Diff::Position.new(new_position)
end
return if new_position == read_attribute(meth)
super(new_position)
end
end
@ -45,7 +48,7 @@ class DiffNote < Note
end
def diff_line
@diff_line ||= diff_file.line_for_position(self.original_position) if diff_file
@diff_line ||= diff_file&.line_for_position(self.original_position)
end
def for_line?(line)

20
app/models/merge_request.rb
View file

@ -416,13 +416,24 @@ class MergeRequest < ActiveRecord::Base
@merge_request_diffs_by_diff_refs_or_sha[diff_refs_or_sha]
end
def version_params_for(diff_refs)
if diff = merge_request_diff_for(diff_refs)
{ diff_id: diff.id }
elsif diff = merge_request_diff_for(diff_refs.head_sha)
{
diff_id: diff.id,
start_sha: diff_refs.start_sha
}
end
end
def reload_diff_if_branch_changed
if source_branch_changed? || target_branch_changed?
reload_diff
end
end
def reload_diff
def reload_diff(current_user = nil)
return unless open?
old_diff_refs = self.diff_refs
@ -432,7 +443,8 @@ class MergeRequest < ActiveRecord::Base
update_diff_notes_positions(
old_diff_refs: old_diff_refs,
new_diff_refs: new_diff_refs
new_diff_refs: new_diff_refs,
current_user: current_user
)
end
@ -861,7 +873,7 @@ class MergeRequest < ActiveRecord::Base
diff_sha_refs && diff_sha_refs.complete?
end
def update_diff_notes_positions(old_diff_refs:, new_diff_refs:)
def update_diff_notes_positions(old_diff_refs:, new_diff_refs:, current_user: nil)
return unless has_complete_diff_refs?
return if new_diff_refs == old_diff_refs
@ -875,7 +887,7 @@ class MergeRequest < ActiveRecord::Base
service = Notes::DiffPositionUpdateService.new(
self.project,
nil,
current_user,
old_diff_refs: old_diff_refs,
new_diff_refs: new_diff_refs,
paths: paths

5
app/models/merge_request_diff.rb
View file

@ -175,12 +175,11 @@ class MergeRequestDiff < ActiveRecord::Base
self == merge_request.merge_request_diff
end
def compare_with(sha, straight: true)
def compare_with(sha)
# When compare merge request versions we want diff A..B instead of A...B
# so we handle cases when user does squash and rebase of the commits between versions.
# For this reason we set straight to true by default.
CompareService.new(project, head_commit_sha)
.execute(project, sha, straight: straight)
CompareService.new(project, head_commit_sha).execute(project, sha, straight: true)
end
def commits_count

11
app/models/note.rb
View file

@ -124,13 +124,12 @@ class Note < ActiveRecord::Base
groups = {}
diff_notes.fresh.discussions.each do |discussion|
if discussion.active?(diff_refs)
discussions = groups[discussion.line_code] ||= []
elsif diff_refs && discussion.created_at_diff?(diff_refs)
discussions = groups[discussion.original_line_code] ||= []
end
line_code = discussion.line_code_in_diffs(diff_refs)
discussions << discussion if discussions
if line_code
discussions = groups[line_code] ||= []
discussions << discussion
end
end
groups

1
app/models/system_note_metadata.rb
View file

@ -2,6 +2,7 @@ class SystemNoteMetadata < ActiveRecord::Base
ICON_TYPES = %w[
commit description merge confidential visible label assignee cross_reference
title time_tracking branch milestone discussion task moved opened closed merged
outdated
].freeze
validates :note, presence: true

4
app/services/merge_requests/refresh_service.rb
View file

@ -66,12 +66,12 @@ module MergeRequests
filter_merge_requests(merge_requests).each do |merge_request|
if merge_request.source_branch == @branch_name || force_push?
merge_request.reload_diff
merge_request.reload_diff(current_user)
else
mr_commit_ids = merge_request.commits_sha
push_commit_ids = @commits.map(&:id)
matches = mr_commit_ids & push_commit_ids
merge_request.reload_diff if matches.any?
merge_request.reload_diff(current_user) if matches.any?
end
merge_request.mark_as_unchecked

2
app/services/merge_requests/reopen_service.rb
View file

@ -8,7 +8,7 @@ module MergeRequests
create_note(merge_request)
notification_service.reopen_mr(merge_request, current_user)
execute_hooks(merge_request, 'reopen')
merge_request.reload_diff
merge_request.reload_diff(current_user)
merge_request.mark_as_unchecked
end

25
app/services/notes/diff_position_update_service.rb
View file

@ -1,26 +1,29 @@
module Notes
class DiffPositionUpdateService < BaseService
def execute(note)
new_position = tracer.trace(note.position)
results = tracer.trace(note.position)
return unless results
# Don't update the position if the type doesn't match, since that means
# the diff line commented on was changed, and the comment is now outdated
old_position = note.position
if new_position &&
new_position != old_position &&
new_position.type == old_position.type
position = results[:position]
outdated = results[:outdated]
note.position = new_position
if outdated
note.change_position = position
if note.persisted? && current_user
SystemNoteService.diff_discussion_outdated(note.to_discussion, project, current_user, position)
end
else
note.position = position
note.change_position = nil
end
note
end
private
def tracer
@tracer ||= Gitlab::Diff::PositionTracer.new(
repository: project.repository,
project: project,
old_diff_refs: params[:old_diff_refs],
new_diff_refs: params[:new_diff_refs],
paths: params[:paths]

24
app/services/system_note_service.rb
View file

@ -258,7 +258,7 @@ module SystemNoteService
create_note(NoteSummary.new(noteable, project, author, body, action: 'title'))
end
def self.resolve_all_discussions(merge_request, project, author)
def resolve_all_discussions(merge_request, project, author)
body = "resolved all discussions"
create_note(NoteSummary.new(merge_request, project, author, body, action: 'discussion'))
@ -274,6 +274,28 @@ module SystemNoteService
note
end
def diff_discussion_outdated(discussion, project, author, change_position)
merge_request = discussion.noteable
diff_refs = change_position.diff_refs
version_index = merge_request.merge_request_diffs.viewable.count
body = "changed this line in"
if version_params = merge_request.version_params_for(diff_refs)
line_code = change_position.line_code(project.repository)
url = url_helpers.diffs_namespace_project_merge_request_url(project.namespace, project, merge_request, version_params.merge(anchor: line_code))
body << " [version #{version_index} of the diff](#{url})"
else
body << " version #{version_index} of the diff"
end
note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
note = Note.create(note_attributes.merge(system: true))
note.system_note_metadata = SystemNoteMetadata.new(action: 'outdated')
note
end
# Called when the title of a Noteable is changed
#
# noteable - Noteable object that responds to `title`

7
app/views/discussions/_discussion.html.haml
View file

@ -32,10 +32,9 @@
- elsif discussion.diff_discussion?
on
= conditional_link_to url.present?, url do
- if discussion.active?
the diff
- else
an outdated diff
- unless discussion.active?
an old version of
the diff
= time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago")
= render "discussions/headline", discussion: discussion

2
app/views/projects/merge_requests/show/_versions.html.haml
View file

@ -91,7 +91,7 @@
comparing two versions
- else
viewing an old version
of this merge request.
of the diff.
.pull-right
= link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'

4
changelogs/unreleased/dm-outdated-system-note.yml Normal file
View file

@ -0,0 +1,4 @@
---
title: Add system note with link to diff comparison when MR discussion becomes outdated
merge_request:
author:

13
db/migrate/20170521184006_add_change_position_to_notes.rb Normal file
View file

@ -0,0 +1,13 @@
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddChangePositionToNotes < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
def change
add_column :notes, :change_position, :text
end
end

3
db/schema.rb
View file

@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20170518231126) do
ActiveRecord::Schema.define(version: 20170521184006) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@ -794,6 +794,7 @@ ActiveRecord::Schema.define(version: 20170518231126) do
t.string "discussion_id"
t.text "note_html"
t.integer "cached_markdown_version"
t.text "change_position"
end
add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree

8
lib/gitlab/diff/file_collection/base.rb
View file

@ -24,6 +24,14 @@ module Gitlab
@diff_files ||= @diffs.decorate! { |diff| decorate_diff!(diff) }
end
def diff_file_with_old_path(old_path)
diff_files.find { |diff_file| diff_file.old_path == old_path }
end
def diff_file_with_new_path(new_path)
diff_files.find { |diff_file| diff_file.new_path == new_path }
end
private
def decorate_diff!(diff)

30
lib/gitlab/diff/position.rb
View file

@ -12,20 +12,26 @@ module Gitlab
attr_reader :head_sha
def initialize(attrs = {})
if diff_file = attrs[:diff_file]
attrs[:diff_refs] = diff_file.diff_refs
attrs[:old_path] = diff_file.old_path
attrs[:new_path] = diff_file.new_path
end
if diff_refs = attrs[:diff_refs]
attrs[:base_sha] = diff_refs.base_sha
attrs[:start_sha] = diff_refs.start_sha
attrs[:head_sha] = diff_refs.head_sha
end
@old_path = attrs[:old_path]
@new_path = attrs[:new_path]
@base_sha = attrs[:base_sha]
@start_sha = attrs[:start_sha]
@head_sha = attrs[:head_sha]
@old_line = attrs[:old_line]
@new_line = attrs[:new_line]
if attrs[:diff_refs]
@base_sha = attrs[:diff_refs].base_sha
@start_sha = attrs[:diff_refs].start_sha
@head_sha = attrs[:diff_refs].head_sha
else
@base_sha = attrs[:base_sha]
@start_sha = attrs[:start_sha]
@head_sha = attrs[:head_sha]
end
end
# `Gitlab::Diff::Position` objects are stored as serialized attributes in
@ -129,11 +135,11 @@ module Gitlab
end
def diff_line(repository)
@diff_line ||= diff_file(repository).line_for_position(self)
@diff_line ||= diff_file(repository)&.line_for_position(self)
end
def line_code(repository)
@line_code ||= diff_file(repository).line_code_for_position(self)
@line_code ||= diff_file(repository)&.line_code_for_position(self)
end
private

240
lib/gitlab/diff/position_tracer.rb
View file

@ -3,21 +3,21 @@
module Gitlab
module Diff
class PositionTracer
attr_accessor :repository
attr_accessor :project
attr_accessor :old_diff_refs
attr_accessor :new_diff_refs
attr_accessor :paths
def initialize(repository:, old_diff_refs:, new_diff_refs:, paths: nil)
@repository = repository
def initialize(project:, old_diff_refs:, new_diff_refs:, paths: nil)
@project = project
@old_diff_refs = old_diff_refs
@new_diff_refs = new_diff_refs
@paths = paths
end
def trace(old_position)
def trace(ab_position)
return unless old_diff_refs&.complete? && new_diff_refs&.complete?
return unless old_position.diff_refs == old_diff_refs
return unless ab_position.diff_refs == old_diff_refs
# Suppose we have an MR with source branch `feature` and target branch `master`.
# When the MR was created, the head of `master` was commit A, and the
@ -44,14 +44,16 @@ module Gitlab
#
# For diff notes for diff A->B, the position looks like this:
# Position
# base_sha - ID of commit A
# start_sha - ID of commit A
# head_sha - ID of commit B
# base_sha - ID of base commit of A and B
# old_path - path as of A (nil if file was newly created)
# new_path - path as of B (nil if file was deleted)
# old_line - line number as of A (nil if file was newly created)
# new_line - line number as of B (nil if file was deleted)
#
# We can easily update `base_sha` and `head_sha` to hold the IDs of commits C and D,
# We can easily update `start_sha` and `head_sha` to hold the IDs of
# commits C and D, and can trivially determine `base_sha` based on those,
# but need to find the paths and line numbers as of C and D.
#
# If the file was unchanged or newly created in A->B, the path as of D can be found
@ -68,108 +70,162 @@ module Gitlab
# by generating diff A->C ("base to base"), finding the diff file with
# `diff_file.old_path == position.old_path`, and taking `diff_file.new_path`.
# The path as of D can be found by taking diff C->D, finding the diff file
# with that same `old_path` and taking `diff_file.new_path`.
# with `old_path` set to that `diff_file.new_path` and taking `diff_file.new_path`.
# The line number as of C can be found by using the LineMapper on diff A->C
# and providing the line number as of A.
# The line number as of D can be found by using the LineMapper on diff C->D
# and providing the line number as of C.
results = nil
results ||= trace_added_line(old_position) if old_position.added? || old_position.unchanged?
results ||= trace_removed_line(old_position) if old_position.removed? || old_position.unchanged?
return unless results
file_diff, old_line, new_line = results
new_position = Position.new(
old_path: file_diff.old_path,
new_path: file_diff.new_path,
head_sha: new_diff_refs.head_sha,
start_sha: new_diff_refs.start_sha,
base_sha: new_diff_refs.base_sha,
old_line: old_line,
new_line: new_line
)
# If a position is found, but is not actually contained in the diff, for example
# because it was an unchanged line in the context of a change that was undone,
# we cannot return this as a successful trace.
return unless new_position.diff_line(repository)
new_position
if ab_position.added?
trace_added_line(ab_position)
elsif ab_position.removed?
trace_removed_line(ab_position)
else # unchanged
trace_unchanged_line(ab_position)
end
end
private
def trace_added_line(old_position)
file_path = old_position.new_path
def trace_added_line(ab_position)
b_path = ab_position.new_path
b_line = ab_position.new_line
return unless diff_head_to_head
bd_diff = bd_diffs.diff_file_with_old_path(b_path)
file_head_to_head = diff_head_to_head.find { |diff_file| diff_file.old_path == file_path }
d_path = bd_diff&.new_path || b_path
d_line = LineMapper.new(bd_diff).old_to_new(b_line)
file_path = file_head_to_head.new_path if file_head_to_head
if d_line
cd_diff = cd_diffs.diff_file_with_new_path(d_path)
new_line = LineMapper.new(file_head_to_head).old_to_new(old_position.new_line)
c_path = cd_diff&.old_path || d_path
c_line = LineMapper.new(cd_diff).new_to_old(d_line)
return unless new_line
if c_line
# If the line is still in D but also in C, it has turned from an
# added line into an unchanged one.
new_position = position(cd_diff, c_line, d_line)
if valid_position?(new_position)
# If the line is still in the MR, we don't treat this as outdated.
{ position: new_position, outdated: false }
else
# If the line is no longer in the MR, we unfortunately cannot show
# the current state on the CD diff, so we treat it as outdated.
ac_diff = ac_diffs.diff_file_with_new_path(c_path)
file_diff = new_diffs.find { |diff_file| diff_file.new_path == file_path }
return unless file_diff
old_line = LineMapper.new(file_diff).new_to_old(new_line)
[file_diff, old_line, new_line]
end
def trace_removed_line(old_position)
file_path = old_position.old_path
return unless diff_base_to_base
file_base_to_base = diff_base_to_base.find { |diff_file| diff_file.old_path == file_path }
file_path = file_base_to_base.old_path if file_base_to_base
old_line = LineMapper.new(file_base_to_base).old_to_new(old_position.old_line)
return unless old_line
file_diff = new_diffs.find { |diff_file| diff_file.old_path == file_path }
return unless file_diff
new_line = LineMapper.new(file_diff).old_to_new(old_line)
[file_diff, old_line, new_line]
end
def diff_base_to_base
@diff_base_to_base ||= diff_files(old_diff_refs.base_sha || old_diff_refs.start_sha, new_diff_refs.base_sha || new_diff_refs.start_sha)
end
def diff_head_to_head
@diff_head_to_head ||= diff_files(old_diff_refs.head_sha, new_diff_refs.head_sha)
end
def new_diffs
@new_diffs ||= diff_files(new_diff_refs.start_sha, new_diff_refs.head_sha, use_base: true)
end
def diff_files(start_sha, head_sha, use_base: false)
base_sha = self.repository.merge_base(start_sha, head_sha) || start_sha
diffs = self.repository.raw_repository.diff(
use_base ? base_sha : start_sha,
head_sha,
{},
*paths
)
diffs.decorate! do |diff|
Gitlab::Diff::File.new(diff, repository: self.repository)
{ position: position(ac_diff, nil, c_line), outdated: true }
end
else
# If the line is still in D and not in C, it is still added.
{ position: position(cd_diff, nil, d_line), outdated: false }
end
else
# If the line is no longer in D, it has been removed from the MR.
{ position: position(bd_diff, b_line, nil), outdated: true }
end
end
def trace_removed_line(ab_position)
a_path = ab_position.old_path
a_line = ab_position.old_line
ac_diff = ac_diffs.diff_file_with_old_path(a_path)
c_path = ac_diff&.new_path || a_path
c_line = LineMapper.new(ac_diff).old_to_new(a_line)
if c_line
cd_diff = cd_diffs.diff_file_with_old_path(c_path)
d_path = cd_diff&.new_path || c_path
d_line = LineMapper.new(cd_diff).old_to_new(c_line)
if d_line
# If the line is still in C but also in D, it has turned from a
# removed line into an unchanged one.
bd_diff = bd_diffs.diff_file_with_new_path(d_path)
{ position: position(bd_diff, nil, d_line), outdated: true }
else
# If the line is still in C and not in D, it is still removed.
{ position: position(cd_diff, c_line, nil), outdated: false }
end
else
# If the line is no longer in C, it has been removed outside of the MR.
{ position: position(ac_diff, a_line, nil), outdated: true }
end
end
def trace_unchanged_line(ab_position)
a_path = ab_position.old_path
a_line = ab_position.old_line
b_path = ab_position.new_path
b_line = ab_position.new_line
ac_diff = ac_diffs.diff_file_with_old_path(a_path)
c_path = ac_diff&.new_path || a_path
c_line = LineMapper.new(ac_diff).old_to_new(a_line)
bd_diff = bd_diffs.diff_file_with_old_path(b_path)
d_line = LineMapper.new(bd_diff).old_to_new(b_line)
cd_diff = cd_diffs.diff_file_with_old_path(c_path)
if c_line && d_line
# If the line is still in C and D, it is still unchanged.
new_position = position(cd_diff, c_line, d_line)
if valid_position?(new_position)
# If the line is still in the MR, we don't treat this as outdated.
{ position: new_position, outdated: false }
else
# If the line is no longer in the MR, we unfortunately cannot show
# the current state on the CD diff or any change on the BD diff,
# so we treat it as outdated.
{ position: nil, outdated: true }
end
elsif d_line # && !c_line
# If the line is still in D but no longer in C, it has turned from
# an unchanged line into an added one.
# We don't treat this as outdated since the line is still in the MR.
{ position: position(cd_diff, nil, d_line), outdated: false }
else # !d_line && (c_line || !c_line)
# If the line is no longer in D, it has turned from an unchanged line
# into a removed one.
{ position: position(bd_diff, b_line, nil), outdated: true }
end
end
def ac_diffs
@ac_diffs ||= compare(
old_diff_refs.base_sha || old_diff_refs.start_sha,
new_diff_refs.base_sha || new_diff_refs.start_sha,
straight: true
)
end
def bd_diffs
@bd_diffs ||= compare(old_diff_refs.head_sha, new_diff_refs.head_sha, straight: true)
end
def cd_diffs
@cd_diffs ||= compare(new_diff_refs.start_sha, new_diff_refs.head_sha)
end
def compare(start_sha, head_sha, straight: false)
compare = CompareService.new(project, head_sha).execute(project, start_sha, straight: straight)
compare.diffs(paths: paths)
end
def position(diff_file, old_line, new_line)
Position.new(diff_file: diff_file, old_line: old_line, new_line: new_line)
end
def valid_position?(position)
!!position.diff_line(project.repository)
end
end
end
end

2
spec/features/merge_requests/discussion_spec.rb
View file

@ -43,7 +43,7 @@ feature 'Merge Request Discussions', feature: true do
it 'shows a link to the outdated diff' do
within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do
path = diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, diff_id: old_merge_request_diff.id, anchor: outdated_discussion.line_code)
expect(page).to have_link('an outdated diff', href: path)
expect(page).to have_link('an old version of the diff', href: path)
end
end
end

2
spec/features/merge_requests/versions_spec.rb
View file

@ -124,6 +124,8 @@ feature 'Merge Request versions', js: true, feature: true do
diff_refs: merge_request_diff3.compare_with(merge_request_diff1.head_commit_sha).diff_refs
)
outdated_diff_note = create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position)
outdated_diff_note.position = outdated_diff_note.original_position
outdated_diff_note.save!
visit current_url
wait_for_requests

8
spec/helpers/notes_helper_spec.rb
View file

@ -92,7 +92,13 @@ describe NotesHelper do
)
end
let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, position: position).to_discussion }
let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, position: position) }
let(:discussion) { diff_note.to_discussion }
before do
diff_note.position = diff_note.original_position
diff_note.save!
end
it 'returns the diff version comparison path with the line code' do
expect(helper.discussion_path(discussion)).to eq(diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, diff_id: merge_request_diff3, start_sha: merge_request_diff1.head_commit_sha, anchor: discussion.line_code))

315
spec/lib/gitlab/diff/position_tracer_spec.rb
View file

@ -61,9 +61,10 @@ describe Gitlab::Diff::PositionTracer, lib: true do
let(:old_diff_refs) { raise NotImplementedError }
let(:new_diff_refs) { raise NotImplementedError }
let(:change_diff_refs) { raise NotImplementedError }
let(:old_position) { raise NotImplementedError }
let(:position_tracer) { described_class.new(repository: project.repository, old_diff_refs: old_diff_refs, new_diff_refs: new_diff_refs) }
let(:position_tracer) { described_class.new(project: project, old_diff_refs: old_diff_refs, new_diff_refs: new_diff_refs) }
subject { position_tracer.trace(old_position) }
def diff_refs(base_commit, head_commit)
@ -77,16 +78,40 @@ describe Gitlab::Diff::PositionTracer, lib: true do
Gitlab::Diff::Position.new(attrs)
end
def expect_new_position(attrs, new_position = subject)
if attrs.nil?
expect(new_position).to be_nil
else
expect(new_position).not_to be_nil
def expect_new_position(attrs, result = subject)
aggregate_failures("expect new position #{attrs.inspect}") do
if attrs.nil?
expect(result[:outdated]).to be_truthy
else
expect(result[:outdated]).to be_falsey
expect(new_position.diff_refs).to eq(new_diff_refs)
new_position = result[:position]
expect(new_position).not_to be_nil
attrs.each do |attr, value|
expect(new_position.send(attr)).to eq(value)
expect(new_position.diff_refs).to eq(new_diff_refs)
attrs.each do |attr, value|
expect(new_position.send(attr)).to eq(value)
end
end
end
end
def expect_change_position(attrs, result = subject)
aggregate_failures("expect change position #{attrs.inspect}") do
expect(result[:outdated]).to be_truthy
change_position = result[:position]
if attrs.nil? || attrs.empty?
expect(change_position).to be_nil
else
expect(change_position).not_to be_nil
expect(change_position.diff_refs).to eq(change_diff_refs)
attrs.each do |attr, value|
expect(change_position.send(attr)).to eq(value)
end
end
end
end
@ -395,6 +420,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when that line was changed between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, update_line_commit) }
let(:change_diff_refs) { diff_refs(create_file_commit, update_line_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -407,14 +433,20 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 + BB
# 3 + C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 2,
new_line: nil
)
end
end
context "when that line was deleted between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, update_line_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, delete_line_commit) }
let(:change_diff_refs) { diff_refs(update_line_commit, delete_line_commit) }
let(:old_position) { position(new_path: file_name, new_line: 3) }
# old diff:
@ -426,8 +458,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 1 + A
# 2 + BB
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 3,
new_line: nil
)
end
end
end
@ -512,6 +549,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when that line was changed between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
let(:new_diff_refs) { diff_refs(create_file_commit, update_line_commit) }
let(:change_diff_refs) { diff_refs(create_file_commit, update_line_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -525,14 +563,20 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 + BB
# 3 3 C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 2,
new_line: nil
)
end
end
context "when that line was deleted between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, move_line_commit) }
let(:new_diff_refs) { diff_refs(move_line_commit, delete_line_commit) }
let(:change_diff_refs) { diff_refs(move_line_commit, delete_line_commit) }
let(:old_position) { position(new_path: file_name, new_line: 3) }
# old diff:
@ -545,8 +589,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 2 A
# 3 - C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 3,
new_line: nil
)
end
end
end
@ -558,6 +607,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when the file's content was unchanged between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, delete_line_commit) }
let(:new_diff_refs) { diff_refs(delete_line_commit, rename_file_commit) }
let(:change_diff_refs) { diff_refs(initial_commit, delete_line_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -569,8 +619,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 1 1 BB
# 2 2 A
it "returns nil since the line doesn't exist in the new diffs anymore" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: nil,
new_line: 2
)
end
end
@ -628,6 +683,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when that line was changed between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, delete_line_commit) }
let(:new_diff_refs) { diff_refs(delete_line_commit, update_line_again_commit) }
let(:change_diff_refs) { diff_refs(delete_line_commit, update_line_again_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -640,28 +696,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 - A
# 2 + AA
it "returns nil" do
expect(subject).to be_nil
end
end
context "when that line was deleted between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, delete_line_commit) }
let(:new_diff_refs) { diff_refs(delete_line_commit, delete_line_again_commit) }
let(:old_position) { position(new_path: file_name, new_line: 1) }
# old diff:
# 1 + BB
# 2 + A
#
# new diff:
# file_name -> new_file_name
# 1 - BB
# 2 - A
# 1 + AA
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: new_file_name,
old_line: 2,
new_line: nil
)
end
end
end
@ -673,6 +714,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when the file's content was unchanged between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, delete_line_commit) }
let(:new_diff_refs) { diff_refs(delete_line_commit, delete_file_commit) }
let(:change_diff_refs) { diff_refs(delete_line_commit, delete_file_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -683,8 +725,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 1 - BB
# 2 - A
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 2,
new_line: nil
)
end
end
@ -692,6 +739,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when that line was unchanged between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, move_line_commit) }
let(:new_diff_refs) { diff_refs(delete_line_commit, delete_file_commit) }
let(:change_diff_refs) { diff_refs(move_line_commit, delete_file_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -703,14 +751,20 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 1 - BB
# 2 - A
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 2,
new_line: nil
)
end
end
context "when that line was moved between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, update_line_commit) }
let(:new_diff_refs) { diff_refs(move_line_commit, delete_file_commit) }
let(:change_diff_refs) { diff_refs(update_line_commit, delete_file_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -723,14 +777,20 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 - A
# 3 - C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 2,
new_line: nil
)
end
end
context "when that line was changed between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
let(:new_diff_refs) { diff_refs(update_line_commit, delete_file_commit) }
let(:change_diff_refs) { diff_refs(create_file_commit, delete_file_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -743,14 +803,20 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 - BB
# 3 - C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 2,
new_line: nil
)
end
end
context "when that line was deleted between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, move_line_commit) }
let(:new_diff_refs) { diff_refs(delete_line_commit, delete_file_commit) }
let(:change_diff_refs) { diff_refs(move_line_commit, delete_file_commit) }
let(:old_position) { position(new_path: file_name, new_line: 3) }
# old diff:
@ -762,8 +828,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 1 - BB
# 2 - A
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 3,
new_line: nil
)
end
end
end
@ -775,6 +846,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when the file's content was unchanged between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
let(:new_diff_refs) { diff_refs(create_file_commit, create_second_file_commit) }
let(:change_diff_refs) { diff_refs(initial_commit, create_file_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -787,8 +859,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 2 B
# 3 3 C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: nil,
new_line: 2
)
end
end
@ -796,6 +873,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when that line was unchanged between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
let(:new_diff_refs) { diff_refs(update_line_commit, update_second_file_line_commit) }
let(:change_diff_refs) { diff_refs(initial_commit, update_line_commit) }
let(:old_position) { position(new_path: file_name, new_line: 1) }
# old diff:
@ -808,14 +886,20 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 2 BB
# 3 3 C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: nil,
new_line: 1
)
end
end
context "when that line was moved between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, update_line_commit) }
let(:new_diff_refs) { diff_refs(move_line_commit, move_second_file_line_commit) }
let(:change_diff_refs) { diff_refs(initial_commit, move_line_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -828,14 +912,20 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 2 A
# 3 3 C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: nil,
new_line: 1
)
end
end
context "when that line was changed between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
let(:new_diff_refs) { diff_refs(update_line_commit, update_second_file_line_commit) }
let(:change_diff_refs) { diff_refs(create_file_commit, update_second_file_line_commit) }
let(:old_position) { position(new_path: file_name, new_line: 2) }
# old diff:
@ -848,14 +938,20 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 2 BB
# 3 3 C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 2,
new_line: nil
)
end
end
context "when that line was deleted between the old and the new diff" do
let(:old_diff_refs) { diff_refs(initial_commit, move_line_commit) }
let(:new_diff_refs) { diff_refs(delete_line_commit, delete_second_file_line_commit) }
let(:change_diff_refs) { diff_refs(move_line_commit, delete_second_file_line_commit) }
let(:old_position) { position(new_path: file_name, new_line: 3) }
# old diff:
@ -867,8 +963,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 1 1 BB
# 2 2 A
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 3,
new_line: nil
)
end
end
end
@ -957,6 +1058,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when that line was changed or deleted between the old and the new diff" do
let(:old_diff_refs) { diff_refs(create_file_commit, move_line_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, create_file_commit) }
let(:change_diff_refs) { diff_refs(move_line_commit, create_file_commit) }
let(:old_position) { position(old_path: file_name, new_path: file_name, new_line: 1) }
# old diff:
@ -970,8 +1072,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 + B
# 3 + C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 1,
new_line: nil
)
end
end
end
@ -980,6 +1087,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when the position pointed at a deleted line in the old diff" do
let(:old_diff_refs) { diff_refs(create_file_commit, update_line_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, update_line_commit) }
let(:change_diff_refs) { diff_refs(create_file_commit, initial_commit) }
let(:old_position) { position(old_path: file_name, new_path: file_name, old_line: 2) }
# old diff:
@ -993,8 +1101,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 + BB
# 3 + C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 2,
new_line: nil
)
end
end
@ -1076,6 +1189,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when that line was changed or deleted between the old and the new diff" do
let(:old_diff_refs) { diff_refs(create_file_commit, move_line_commit) }
let(:new_diff_refs) { diff_refs(initial_commit, delete_line_commit) }
let(:change_diff_refs) { diff_refs(move_line_commit, delete_line_commit) }
let(:old_position) { position(old_path: file_name, new_path: file_name, old_line: 3, new_line: 3) }
# old diff:
@ -1088,8 +1202,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 1 + A
# 2 + B
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 3,
new_line: nil
)
end
end
end
@ -1182,6 +1301,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
context "when that line was changed or deleted between the old and the new diff" do
let(:old_diff_refs) { diff_refs(create_file_commit, move_line_commit) }
let(:new_diff_refs) { diff_refs(create_file_commit, update_line_commit) }
let(:change_diff_refs) { diff_refs(move_line_commit, update_line_commit) }
let(:old_position) { position(old_path: file_name, new_path: file_name, new_line: 1) }
# old diff:
@ -1196,8 +1316,13 @@ describe Gitlab::Diff::PositionTracer, lib: true do
# 2 + BB
# 3 3 C
it "returns nil" do
expect(subject).to be_nil
it "returns the position of the change" do
expect_change_position(
old_path: file_name,
new_path: file_name,
old_line: 1,
new_line: nil
)
end
end
end
@ -1239,7 +1364,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
describe "typical use scenarios" do
let(:second_branch_name) { "#{branch_name}-2" }
def expect_positions(old_attrs, new_attrs)
def expect_new_positions(old_attrs, new_attrs)
old_positions = old_attrs.map do |old_attrs|
position(old_attrs)
end
@ -1248,8 +1373,14 @@ describe Gitlab::Diff::PositionTracer, lib: true do
position_tracer.trace(old_position)
end
new_positions.zip(new_attrs).each do |new_position, new_attrs|
expect_new_position(new_attrs, new_position)
aggregate_failures do
new_positions.zip(new_attrs).each do |new_position, new_attrs|
if new_attrs&.delete(:change)
expect_change_position(new_attrs, new_position)
else
expect_new_position(new_attrs, new_position)
end
end
end
end
@ -1330,6 +1461,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
describe "simple push of new commit" do
let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
let(:new_diff_refs) { diff_refs(create_file_commit, update_file_again_commit) }
let(:change_diff_refs) { diff_refs(update_file_commit, update_file_again_commit) }
# old diff:
# 1 1 A
@ -1368,14 +1500,14 @@ describe Gitlab::Diff::PositionTracer, lib: true do
{ old_path: file_name, new_path: file_name, old_line: 1, new_line: 1 },
{ old_path: file_name, old_line: 2 },
{ old_path: file_name, new_path: file_name, old_line: 3, new_line: 3 },
{ old_path: file_name, old_line: 4, new_line: 4 },
nil,
{ new_path: file_name, new_line: 4, change: true },
{ new_path: file_name, old_line: 3, change: true },
{ old_path: file_name, new_path: file_name, old_line: 5, new_line: 5 },
{ old_path: file_name, old_line: 6 },
{ new_path: file_name, old_line: 5, change: true },
{ new_path: file_name, new_line: 7 }
]
expect_positions(old_position_attrs, new_position_attrs)
expect_new_positions(old_position_attrs, new_position_attrs)
end
end
@ -1402,6 +1534,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
let(:new_diff_refs) { diff_refs(create_file_commit, second_create_file_commit) }
let(:change_diff_refs) { diff_refs(update_file_commit, second_create_file_commit) }
# old diff:
# 1 1 A
@ -1440,20 +1573,21 @@ describe Gitlab::Diff::PositionTracer, lib: true do
{ old_path: file_name, new_path: file_name, old_line: 1, new_line: 1 },
{ old_path: file_name, old_line: 2 },
{ old_path: file_name, new_path: file_name, old_line: 3, new_line: 3 },
{ old_path: file_name, old_line: 4, new_line: 4 },
nil,
{ new_path: file_name, new_line: 4, change: true },
{ old_path: file_name, old_line: 3, change: true },
{ old_path: file_name, new_path: file_name, old_line: 5, new_line: 5 },
{ old_path: file_name, old_line: 6 },
{ old_path: file_name, old_line: 5, change: true },
{ new_path: file_name, new_line: 7 }
]
expect_positions(old_position_attrs, new_position_attrs)
expect_new_positions(old_position_attrs, new_position_attrs)
end
end
describe "force push to delete last commit" do
let(:old_diff_refs) { diff_refs(create_file_commit, update_file_again_commit) }
let(:new_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
let(:change_diff_refs) { diff_refs(update_file_again_commit, update_file_commit) }
# old diff:
# 1 1 A
@ -1492,16 +1626,16 @@ describe Gitlab::Diff::PositionTracer, lib: true do
new_position_attrs = [
{ old_path: file_name, new_path: file_name, old_line: 1, new_line: 1 },
{ old_path: file_name, old_line: 2 },
nil,
{ old_path: file_name, old_line: 2, change: true },
{ old_path: file_name, new_path: file_name, old_line: 3, new_line: 2 },
{ old_path: file_name, old_line: 4 },
{ old_path: file_name, old_line: 4, change: true },
{ old_path: file_name, new_path: file_name, old_line: 5, new_line: 4 },
{ old_path: file_name, new_path: file_name, old_line: 6, new_line: 5 },
nil,
{ new_path: file_name, new_line: 5, change: true },
{ old_path: file_name, old_line: 6, change: true },
{ new_path: file_name, new_line: 6 }
]
expect_positions(old_position_attrs, new_position_attrs)
expect_new_positions(old_position_attrs, new_position_attrs)
end
end
@ -1567,6 +1701,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
let(:old_diff_refs) { diff_refs(create_file_commit, update_file_again_commit) }
let(:new_diff_refs) { diff_refs(create_file_commit, overwrite_update_file_again_commit) }
let(:change_diff_refs) { diff_refs(update_file_again_commit, overwrite_update_file_again_commit) }
# old diff:
# 1 1 A
@ -1618,7 +1753,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
{ new_path: file_name, new_line: 10 }, # + G
]
expect_positions(old_position_attrs, new_position_attrs)
expect_new_positions(old_position_attrs, new_position_attrs)
end
end
@ -1643,6 +1778,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
let(:old_diff_refs) { diff_refs(create_file_commit, update_file_again_commit) }
let(:new_diff_refs) { diff_refs(create_file_commit, merge_commit) }
let(:change_diff_refs) { diff_refs(update_file_again_commit, merge_commit) }
# old diff:
# 1 1 A
@ -1694,13 +1830,14 @@ describe Gitlab::Diff::PositionTracer, lib: true do
{ new_path: file_name, new_line: 10 }, # + G
]
expect_positions(old_position_attrs, new_position_attrs)
expect_new_positions(old_position_attrs, new_position_attrs)
end
end
describe "changing target branch" do
let(:old_diff_refs) { diff_refs(create_file_commit, update_file_again_commit) }
let(:new_diff_refs) { diff_refs(update_file_commit, update_file_again_commit) }
let(:change_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
# old diff:
# 1 1 A
@ -1739,7 +1876,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
new_position_attrs = [
{ old_path: file_name, new_path: file_name, old_line: 1, new_line: 1 },
nil,
{ old_path: file_name, old_line: 2, change: true },
{ new_path: file_name, new_line: 2 },
{ old_path: file_name, new_path: file_name, old_line: 2, new_line: 3 },
{ new_path: file_name, new_line: 4 },
@ -1749,7 +1886,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
{ new_path: file_name, new_line: 7 }
]
expect_positions(old_position_attrs, new_position_attrs)
expect_new_positions(old_position_attrs, new_position_attrs)
end
end
end

1
spec/lib/gitlab/import_export/safe_model_attributes.yml
View file

@ -54,6 +54,7 @@ Note:
- type
- position
- original_position
- change_position
- resolved_at
- resolved_by_id
- discussion_id

26
spec/models/concerns/discussion_on_diff_spec.rb
View file

@ -21,4 +21,30 @@ describe DiscussionOnDiff, model: true do
end
end
end
describe '#line_code_in_diffs' do
context 'when the discussion is active in the diff' do
let(:diff_refs) { subject.position.diff_refs }
it 'returns the current line code' do
expect(subject.line_code_in_diffs(diff_refs)).to eq(subject.line_code)
end
end
context 'when the discussion was created in the diff' do
let(:diff_refs) { subject.original_position.diff_refs }
it 'returns the original line code' do
expect(subject.line_code_in_diffs(diff_refs)).to eq(subject.original_line_code)
end
end
context 'when the discussion is unrelated to the diff' do
let(:diff_refs) { subject.project.commit(RepoHelpers.sample_commit.id).diff_refs }
it 'returns nil' do
expect(subject.line_code_in_diffs(diff_refs)).to be_nil
end
end
end
end

7
spec/models/diff_discussion_spec.rb
View file

@ -48,7 +48,7 @@ describe DiffDiscussion, model: true do
end
it 'returns the diff ID for the version to show' do
expect(diff_id: merge_request_diff1.id)
expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff1.id)
end
end
@ -65,6 +65,11 @@ describe DiffDiscussion, model: true do
let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, position: position) }
before do
diff_note.position = diff_note.original_position
diff_note.save!
end
it 'returns the diff ID and start sha of the versions to compare' do
expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha)
end

36
spec/models/merge_request_spec.rb
View file

@ -1213,7 +1213,7 @@ describe MergeRequest, models: true do
expect(Notes::DiffPositionUpdateService).to receive(:new).with(
subject.project,
nil,
subject.author,
old_diff_refs: old_diff_refs,
new_diff_refs: commit.diff_refs,
paths: note.position.paths
@ -1222,7 +1222,7 @@ describe MergeRequest, models: true do
expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note)
expect_any_instance_of(DiffNote).to receive(:save).once
subject.reload_diff
subject.reload_diff(subject.author)
end
end
@ -1534,4 +1534,36 @@ describe MergeRequest, models: true do
end
end
end
describe '#version_params_for' do
subject { create(:merge_request, importing: true) }
let(:project) { subject.project }
let!(:merge_request_diff1) { subject.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }
let!(:merge_request_diff2) { subject.merge_request_diffs.create(head_commit_sha: nil) }
let!(:merge_request_diff3) { subject.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
context 'when the diff refs are for an older merge request version' do
let(:diff_refs) { merge_request_diff1.diff_refs }
it 'returns the diff ID for the version to show' do
expect(subject.version_params_for(diff_refs)).to eq(diff_id: merge_request_diff1.id)
end
end
context 'when the diff refs are for a comparison between merge request versions' do
let(:diff_refs) { merge_request_diff3.compare_with(merge_request_diff1.head_commit_sha).diff_refs }
it 'returns the diff ID and start sha of the versions to compare' do
expect(subject.version_params_for(diff_refs)).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha)
end
end
context 'when the diff refs are not for a merge request version' do
let(:diff_refs) { project.commit(sample_commit.id).diff_refs }
it 'returns nil' do
expect(subject.version_params_for(diff_refs)).to be_nil
end
end
end
end

20
spec/services/notes/diff_position_update_service_spec.rb
View file

@ -2,6 +2,7 @@ require 'spec_helper'
describe Notes::DiffPositionUpdateService, services: true do
let(:project) { create(:project, :repository) }
let(:current_user) { project.owner }
let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") }
let(:modify_commit) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e") }
let(:edit_commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") }
@ -25,7 +26,7 @@ describe Notes::DiffPositionUpdateService, services: true do
subject do
described_class.new(
project,
nil,
current_user,
old_diff_refs: old_diff_refs,
new_diff_refs: new_diff_refs,
paths: [path]
@ -170,6 +171,23 @@ describe Notes::DiffPositionUpdateService, services: true do
expect(note.original_position).to eq(old_position)
expect(note.position).to eq(old_position)
end
it 'sets the change position' do
subject.execute(note)
change_position = note.change_position
expect(change_position.start_sha).to eq(old_diff_refs.head_sha)
expect(change_position.head_sha).to eq(new_diff_refs.head_sha)
expect(change_position.old_line).to eq(9)
expect(change_position.new_line).to be_nil
end
it 'creates a system note' do
expect(SystemNoteService).to receive(:diff_discussion_outdated).with(
note.to_discussion, project, current_user, instance_of(Gitlab::Diff::Position))
subject.execute(note)
end
end
end
end

31
spec/services/system_note_service_spec.rb
View file

@ -1034,4 +1034,35 @@ describe SystemNoteService, services: true do
expect(subject.note).to eq 'resolved all discussions'
end
end
describe '.diff_discussion_outdated' do
let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
let(:merge_request) { discussion.noteable }
let(:project) { merge_request.source_project }
let(:change_position) { discussion.position }
def reloaded_merge_request
MergeRequest.find(merge_request.id)
end
subject { described_class.diff_discussion_outdated(discussion, project, author, change_position) }
it_behaves_like 'a system note' do
let(:expected_noteable) { discussion.first_note.noteable }
let(:action) { 'outdated' }
end
it 'creates a new note in the discussion' do
# we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
expect { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
end
it 'links to the diff in the system note' do
expect(subject.note).to include('version 1')
diff_id = merge_request.merge_request_diff.id
line_code = change_position.line_code(project.repository)
expect(subject.note).to include(diffs_namespace_project_merge_request_url(project.namespace, project, merge_request, diff_id: diff_id, anchor: line_code))
end
end
end