summaryrefslogtreecommitdiff
path: root/app
diff options
context:
space:
mode:
authorDouwe Maan <douwe@selenight.nl>2017-05-21 15:38:33 -0500
committerDouwe Maan <douwe@selenight.nl>2017-05-23 16:27:30 -0500
commitab91f76e8b275d0fc1d78b38b95d4b2cd8e73bdb (patch)
treee22910b5a3fc8dee98c5d674ee030b46421a4727 /app
parent52527be4387cb978402a330f2e4de96e586a62db (diff)
downloadgitlab-ce-ab91f76e8b275d0fc1d78b38b95d4b2cd8e73bdb.tar.gz
Add system note with link to diff comparison when MR discussion becomes outdated
Diffstat (limited to 'app')
-rw-r--r--app/assets/stylesheets/framework/timeline.scss2
-rw-r--r--app/assets/stylesheets/pages/note_form.scss4
-rw-r--r--app/assets/stylesheets/pages/notes.scss19
-rw-r--r--app/helpers/system_note_helper.rb3
-rw-r--r--app/models/diff_discussion.rb16
-rw-r--r--app/models/diff_note.rb7
-rw-r--r--app/models/merge_request.rb20
-rw-r--r--app/models/merge_request_diff.rb5
-rw-r--r--app/models/note.rb17
-rw-r--r--app/models/system_note_metadata.rb1
-rw-r--r--app/services/merge_requests/refresh_service.rb4
-rw-r--r--app/services/merge_requests/reopen_service.rb2
-rw-r--r--app/services/notes/diff_position_update_service.rb25
-rw-r--r--app/services/system_note_service.rb24
-rw-r--r--app/views/discussions/_discussion.html.haml7
-rw-r--r--app/views/projects/merge_requests/show/_versions.html.haml2
16 files changed, 97 insertions, 61 deletions
diff --git a/app/assets/stylesheets/framework/timeline.scss b/app/assets/stylesheets/framework/timeline.scss
index aa0c512a277..515e6f0b2b1 100644
--- a/app/assets/stylesheets/framework/timeline.scss
+++ b/app/assets/stylesheets/framework/timeline.scss
@@ -5,7 +5,7 @@
.note-text {
p:last-child {
- margin-bottom: 0;
+ margin-bottom: 0 !important;
}
}
diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss
index 9db26f99a75..49e453c7dbc 100644
--- a/app/assets/stylesheets/pages/note_form.scss
+++ b/app/assets/stylesheets/pages/note_form.scss
@@ -164,10 +164,6 @@
.discussion-body,
.diff-file {
- .notes .note {
- padding: 10px 15px;
- }
-
.discussion-reply-holder {
background-color: $white-light;
padding: 10px 16px;
diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index 4b15fc2bd82..5550c0d5b47 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -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 {
diff --git a/app/helpers/system_note_helper.rb b/app/helpers/system_note_helper.rb
index d889d141101..209bd56b78a 100644
--- a/app/helpers/system_note_helper.rb
+++ b/app/helpers/system_note_helper.rb
@@ -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)
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 14ddd2fcc88..800574d8be0 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -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
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 76c59199afd..1764004078e 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -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)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 9be00880438..2eec013fa9d 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -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
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index f0a3c30ea74..6e3917a10a3 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -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
diff --git a/app/models/note.rb b/app/models/note.rb
index 46d0a4f159f..b4228cc6492 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -121,16 +121,17 @@ class Note < ActiveRecord::Base
end
def grouped_diff_discussions(diff_refs = nil)
- groups = {}
+ groups = Hash.new { |h, k| h[k] = [] }
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
-
- discussions << discussion if discussions
+ line_code =
+ if discussion.active?(diff_refs)
+ discussion.line_code
+ elsif diff_refs && discussion.created_at_diff?(diff_refs)
+ discussion.original_line_code
+ end
+
+ groups[line_code] << discussion if line_code
end
groups
diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb
index b44f4fe000c..414c95f7705 100644
--- a/app/models/system_note_metadata.rb
+++ b/app/models/system_note_metadata.rb
@@ -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
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 1131d6f4913..81d217929d5 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -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
diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb
index fadcce5d9b6..54b19e6d651 100644
--- a/app/services/merge_requests/reopen_service.rb
+++ b/app/services/merge_requests/reopen_service.rb
@@ -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
diff --git a/app/services/notes/diff_position_update_service.rb b/app/services/notes/diff_position_update_service.rb
index 0cb731f5bc3..eff7b287269 100644
--- a/app/services/notes/diff_position_update_service.rb
+++ b/app/services/notes/diff_position_update_service.rb
@@ -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
- end
+ if outdated
+ note.change_position = position
- note
+ 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
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]
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 93bf1fb1615..0837c07e6aa 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -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`
diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml
index 74992e439f3..578e751ab47 100644
--- a/app/views/discussions/_discussion.html.haml
+++ b/app/views/discussions/_discussion.html.haml
@@ -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
diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml
index 37117bc64a3..0999b95c9c9 100644
--- a/app/views/projects/merge_requests/show/_versions.html.haml
+++ b/app/views/projects/merge_requests/show/_versions.html.haml
@@ -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'