summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/helpers/notes_helper.rb4
-rw-r--r--app/models/concerns/resolvable_discussion.rb1
-rw-r--r--app/models/concerns/resolvable_note.rb24
-rw-r--r--app/models/merge_request.rb6
-rw-r--r--app/services/discussions/update_diff_position_service.rb4
-rw-r--r--app/views/discussions/_headline.html.haml23
-rw-r--r--app/views/projects/_merge_request_merge_settings.html.haml2
-rw-r--r--db/migrate/20170905112933_add_resolved_by_push_to_notes.rb9
-rw-r--r--db/schema.rb3
-rw-r--r--doc/user/discussions/img/automatically_resolve_outdated_diff_discussions.pngbin107568 -> 0 bytes
-rw-r--r--doc/user/discussions/img/automatically_resolve_outdated_discussions.pngbin0 -> 117604 bytes
-rw-r--r--doc/user/discussions/index.md18
-rw-r--r--spec/features/merge_requests/resolve_outdated_diff_discussions.rb100
-rw-r--r--spec/helpers/notes_helper_spec.rb20
-rw-r--r--spec/models/merge_request_spec.rb38
-rw-r--r--spec/services/discussions/update_diff_position_service_spec.rb62
16 files changed, 212 insertions, 102 deletions
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 8c5e258f519..083ace65b09 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -146,4 +146,8 @@ module NotesHelper
autocomplete: autocomplete
}
end
+
+ def discussion_resolved_intro(discussion)
+ discussion.resolved_by_push? ? 'Automatically resolved' : 'Resolved'
+ end
end
diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index dd979e7bb17..f006a271327 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -24,6 +24,7 @@ module ResolvableDiscussion
delegate :resolved_at,
:resolved_by,
+ :resolved_by_push?,
to: :last_resolved_note,
allow_nil: true
diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb
index 05eb6f86704..6f39029ff5a 100644
--- a/app/models/concerns/resolvable_note.rb
+++ b/app/models/concerns/resolvable_note.rb
@@ -51,22 +51,30 @@ module ResolvableNote
end
# If you update this method remember to also update `.resolve!`
- def resolve!(current_user)
- return unless resolvable?
- return if resolved?
+ def resolve_without_save(current_user, resolved_by_push: false)
+ return false unless resolvable?
+ return false if resolved?
self.resolved_at = Time.now
self.resolved_by = current_user
- save!
+ self.resolved_by_push = resolved_by_push
end
# If you update this method remember to also update `.unresolve!`
- def unresolve!
- return unless resolvable?
- return unless resolved?
+ def unresolve_without_save(current_user)
+ return false unless resolvable?
+ return false unless resolved?
self.resolved_at = nil
self.resolved_by = nil
- save!
+ end
+
+ def resolve!(current_user, resolved_by_push: false)
+ resolve_without_save(current_user, resolved_by_push: resolved_by_push) &&
+ save!
+ end
+
+ def unresolve!
+ unresolve_without_save(current_user) && save
end
end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 724fb4ccef1..b82f49d7073 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -918,6 +918,12 @@ class MergeRequest < ActiveRecord::Base
active_diff_discussions.each do |discussion|
service.execute(discussion)
end
+
+ if project.resolve_outdated_diff_discussions?
+ MergeRequests::ResolvedDiscussionNotificationService
+ .new(project, current_user)
+ .execute(self)
+ end
end
def keep_around_commit
diff --git a/app/services/discussions/update_diff_position_service.rb b/app/services/discussions/update_diff_position_service.rb
index 1ef8d9edbe1..746f209e20f 100644
--- a/app/services/discussions/update_diff_position_service.rb
+++ b/app/services/discussions/update_diff_position_service.rb
@@ -10,6 +10,10 @@ module Discussions
discussion.notes.each do |note|
if outdated
note.change_position = position
+
+ if project.resolve_outdated_diff_discussions?
+ note.resolve_without_save(current_user, resolved_by_push: true)
+ end
else
note.position = position
note.change_position = nil
diff --git a/app/views/discussions/_headline.html.haml b/app/views/discussions/_headline.html.haml
index a9fe805bfa2..b865eb815f0 100644
--- a/app/views/discussions/_headline.html.haml
+++ b/app/views/discussions/_headline.html.haml
@@ -1,19 +1,12 @@
- if discussion.resolved?
- - if project.resolve_outdated_diff_discussions
- .discussion-headline-light.js-discussion-headline
- Automatically resolved
- - if discussion.resolved_by
- by
- = link_to_member(@project, discussion.resolved_by, avatar: false)
- with a push
- = time_ago_with_tooltip(discussion.resolved_at, placement: "bottom")
- - else
- .discussion-headline-light.js-discussion-headline
- Resolved
- - if discussion.resolved_by
- by
- = link_to_member(@project, discussion.resolved_by, avatar: false)
- = time_ago_with_tooltip(discussion.resolved_at, placement: "bottom")
+ .discussion-headline-light.js-discussion-headline
+ = discussion_resolved_intro(discussion)
+ - if discussion.resolved_by
+ by
+ = link_to_member(@project, discussion.resolved_by, avatar: false)
+ - if discussion.resolved_by_push?
+ with a push
+ = time_ago_with_tooltip(discussion.resolved_at, placement: "bottom")
- elsif discussion.updated?
.discussion-headline-light.js-discussion-headline
Last updated
diff --git a/app/views/projects/_merge_request_merge_settings.html.haml b/app/views/projects/_merge_request_merge_settings.html.haml
index c1cb3b07d8e..e3effe45d6c 100644
--- a/app/views/projects/_merge_request_merge_settings.html.haml
+++ b/app/views/projects/_merge_request_merge_settings.html.haml
@@ -16,7 +16,7 @@
.checkbox
= form.label :resolve_outdated_diff_discussions do
= form.check_box :resolve_outdated_diff_discussions
- %strong Automatically resolve merge request diffs discussions on lines changed with a push
+ %strong Automatically resolve merge request diff discussions when they become outdated
.checkbox
= form.label :printing_merge_request_link_enabled do
= form.check_box :printing_merge_request_link_enabled
diff --git a/db/migrate/20170905112933_add_resolved_by_push_to_notes.rb b/db/migrate/20170905112933_add_resolved_by_push_to_notes.rb
new file mode 100644
index 00000000000..ceb31ffb08a
--- /dev/null
+++ b/db/migrate/20170905112933_add_resolved_by_push_to_notes.rb
@@ -0,0 +1,9 @@
+class AddResolvedByPushToNotes < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ def change
+ add_column :notes, :resolved_by_push, :boolean
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 981ce5425d3..69911ce7107 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20170901071411) do
+ActiveRecord::Schema.define(version: 20170905112933) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@@ -1002,6 +1002,7 @@ ActiveRecord::Schema.define(version: 20170901071411) do
t.text "note_html"
t.integer "cached_markdown_version"
t.text "change_position"
+ t.boolean "resolved_by_push"
end
add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree
diff --git a/doc/user/discussions/img/automatically_resolve_outdated_diff_discussions.png b/doc/user/discussions/img/automatically_resolve_outdated_diff_discussions.png
deleted file mode 100644
index 500c24ee175..00000000000
--- a/doc/user/discussions/img/automatically_resolve_outdated_diff_discussions.png
+++ /dev/null
Binary files differ
diff --git a/doc/user/discussions/img/automatically_resolve_outdated_discussions.png b/doc/user/discussions/img/automatically_resolve_outdated_discussions.png
new file mode 100644
index 00000000000..9a798ddd178
--- /dev/null
+++ b/doc/user/discussions/img/automatically_resolve_outdated_discussions.png
Binary files differ
diff --git a/doc/user/discussions/index.md b/doc/user/discussions/index.md
index b2264c15d63..efea99eb120 100644
--- a/doc/user/discussions/index.md
+++ b/doc/user/discussions/index.md
@@ -116,17 +116,22 @@ are resolved.
![Only allow merge if all the discussions are resolved message](img/only_allow_merge_if_all_discussions_are_resolved_msg.png)
-### Automatically resolve merge request diffs discussions on lines changed with a push
+### Automatically resolve merge request diff discussions when they become outdated
-You can automatically resolve merge request diff discussions on lines modified with a new commit.
+> [Introduced][ce-14053] in GitLab 10.0.
-Navigate to your project's settings page, select the
-**Automatically resolve merge request diffs discussions on lines changed with a push** check box and hit
+You can automatically resolve merge request diff discussions on lines modified
+with a new push.
+
+Navigate to your project's settings page, select the **Automatically resolve
+merge request diffs discussions on lines changed with a push** check box and hit
**Save** for the changes to take effect.
-![Automatically resolve merge request diffs discussions on lines changed with a push](img/automatically_resolve_outdated_diff_discussions.png)
+![Automatically resolve merge request diff discussions when they become outdated](img/automatically_resolve_outdated_discussions.png)
-From now on, any discussions on an outdated diff will be resolved by default.
+From now on, any discussions on a diff will be resolved by default if a push
+makes that diff section outdated. Discussions on lines that don't change and
+top-level resolvable discussions are not automatically resolved.
## Threaded discussions
@@ -153,6 +158,7 @@ comments in greater detail.
[ce-7527]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7527
[ce-7180]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7180
[ce-8266]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8266
+[ce-14053]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14053
[resolve-discussion-button]: img/resolve_discussion_button.png
[resolve-comment-button]: img/resolve_comment_button.png
[discussion-view]: img/discussion_view.png
diff --git a/spec/features/merge_requests/resolve_outdated_diff_discussions.rb b/spec/features/merge_requests/resolve_outdated_diff_discussions.rb
index fd9b2c95210..55a82bdf2b9 100644
--- a/spec/features/merge_requests/resolve_outdated_diff_discussions.rb
+++ b/spec/features/merge_requests/resolve_outdated_diff_discussions.rb
@@ -1,80 +1,78 @@
require 'spec_helper'
feature 'Resolve outdated diff discussions', js: true do
- let(:merge_request) { create(:merge_request, importing: true) }
- let(:project) { merge_request.source_project }
+ let(:project) { create(:project, :repository, :public) }
- 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(:merge_request) do
+ create(:merge_request, source_project: project, source_branch: 'csv', target_branch: 'master')
+ end
+
+ let(:outdated_diff_refs) { project.commit('926c6595b263b2a40da6b17f3e3b7ea08344fad6').diff_refs }
+ let(:current_diff_refs) { merge_request.diff_refs }
let(:outdated_position) do
Gitlab::Diff::Position.new(
- old_path: "files/ruby/popen.rb",
- new_path: "files/ruby/popen.rb",
+ old_path: 'files/csv/Book1.csv',
+ new_path: 'files/csv/Book1.csv',
old_line: nil,
new_line: 9,
diff_refs: outdated_diff_refs
)
end
- let(:outdated_diff_refs) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e").diff_refs }
-
- before do
- sign_in(create(:admin))
+ let(:current_position) do
+ Gitlab::Diff::Position.new(
+ old_path: 'files/csv/Book1.csv',
+ new_path: 'files/csv/Book1.csv',
+ old_line: nil,
+ new_line: 1,
+ diff_refs: current_diff_refs
+ )
end
- context 'when project.resolve_outdated_diff_discussions == true' do
- before do
- project.update_column(:resolve_outdated_diff_discussions, true)
- end
+ let!(:outdated_discussion) do
+ create(:diff_note_on_merge_request,
+ project: project,
+ noteable: merge_request,
+ position: outdated_position).to_discussion
+ end
- context 'with unresolved outdated discussions' do
- it 'does not show outdated discussion' do
- visit_merge_request(merge_request)
- within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do
- expect(page).to have_css('.discussion-body .hide .js-toggle-content', visible: false)
- expect(page).to have_content('Automatically resolved')
- end
- end
- end
+ let!(:current_discussion) do
+ create(:diff_note_on_merge_request,
+ noteable: merge_request,
+ project: project,
+ position: current_position).to_discussion
+ end
- context 'with unresolved active discussions' do
- it 'shows active discussion' do
- visit_merge_request(merge_request)
- within(".discussion[data-discussion-id='#{active_discussion.id}']") do
- expect(page).to have_css('.discussion-body .hide .js-toggle-content', visible: true)
- expect(page).not_to have_content('Automatically resolved')
- end
- end
- end
+ before do
+ sign_in(merge_request.author)
end
- context 'when project.resolve_outdated_diff_discussions == false' do
+ context 'when a discussion was resolved by a push' do
before do
- project.update_column(:resolve_outdated_diff_discussions, false)
+ project.update!(resolve_outdated_diff_discussions: true)
+
+ merge_request.update_diff_discussion_positions(
+ old_diff_refs: outdated_diff_refs,
+ new_diff_refs: current_diff_refs,
+ current_user: merge_request.author
+ )
+
+ visit project_merge_request_path(project, merge_request)
end
- context 'with unresolved outdated discussions' do
- it 'shows outdated discussion' do
- visit_merge_request(merge_request)
- within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do
- expect(page).to have_css('.discussion-body .hide .js-toggle-content', visible: true)
- expect(page).not_to have_content('Automatically resolved')
- end
+ it 'shows that as automatically resolved' do
+ within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do
+ expect(page).to have_css('.discussion-body', visible: false)
+ expect(page).to have_content('Automatically resolved')
end
end
- context 'with unresolved active discussions' do
- it 'shows active discussion' do
- visit_merge_request(merge_request)
- within(".discussion[data-discussion-id='#{active_discussion.id}']") do
- expect(page).to have_css('.discussion-body .hide .js-toggle-content', visible: true)
- expect(page).not_to have_content('Automatically resolved')
- end
+ it 'does not show that for active discussions' do
+ within(".discussion[data-discussion-id='#{current_discussion.id}']") do
+ expect(page).to have_css('.discussion-body', visible: true)
+ expect(page).not_to have_content('Automatically resolved')
end
end
end
- def visit_merge_request(merge_request)
- visit project_merge_request_path(project, merge_request)
- end
end
diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb
index 9921ca1af33..68540dd4e59 100644
--- a/spec/helpers/notes_helper_spec.rb
+++ b/spec/helpers/notes_helper_spec.rb
@@ -231,7 +231,7 @@ describe NotesHelper do
end
end
- describe '#form_resurces' do
+ describe '#form_resources' do
it 'returns note for personal snippet' do
@snippet = create(:personal_snippet)
@note = create(:note_on_personal_snippet)
@@ -266,4 +266,22 @@ describe NotesHelper do
expect(noteable_note_url(note)).to match("/#{project.namespace.path}/#{project.path}/issues/#{issue.iid}##{dom_id(note)}")
end
end
+
+ describe '#discussion_resolved_intro' do
+ context 'when the discussion was resolved by a push' do
+ let(:discussion) { double(:discussion, resolved_by_push?: true) }
+
+ it 'returns "Automatically resolved"' do
+ expect(discussion_resolved_intro(discussion)).to eq('Automatically resolved')
+ end
+ end
+
+ context 'when the discussion was not resolved by a push' do
+ let(:discussion) { double(:discussion, resolved_by_push?: false) }
+
+ it 'returns "Resolved"' do
+ expect(discussion_resolved_intro(discussion)).to eq('Resolved')
+ end
+ end
+ end
end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index f5d079c27c4..d80d5657c42 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1262,7 +1262,6 @@ describe MergeRequest do
describe "#reload_diff" do
let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion }
-
let(:commit) { subject.project.commit(sample_commit.id) }
it "does not change existing merge request diff" do
@@ -1280,9 +1279,19 @@ describe MergeRequest do
subject.reload_diff
end
- it "updates diff discussion positions" do
- old_diff_refs = subject.diff_refs
+ it "calls update_diff_discussion_positions" do
+ expect(subject).to receive(:update_diff_discussion_positions)
+
+ subject.reload_diff
+ end
+ end
+ describe '#update_diff_discussion_positions' do
+ let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion }
+ let(:commit) { subject.project.commit(sample_commit.id) }
+ let(:old_diff_refs) { subject.diff_refs }
+
+ before do
# Update merge_request_diff so that #diff_refs will return commit.diff_refs
allow(subject).to receive(:create_merge_request_diff) do
subject.merge_request_diffs.create(
@@ -1293,7 +1302,9 @@ describe MergeRequest do
subject.merge_request_diff(true)
end
+ end
+ it "updates diff discussion positions" do
expect(Discussions::UpdateDiffPositionService).to receive(:new).with(
subject.project,
subject.author,
@@ -1305,7 +1316,26 @@ describe MergeRequest do
expect_any_instance_of(Discussions::UpdateDiffPositionService).to receive(:execute).with(discussion).and_call_original
expect_any_instance_of(DiffNote).to receive(:save).once
- subject.reload_diff(subject.author)
+ subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
+ new_diff_refs: commit.diff_refs,
+ current_user: subject.author)
+ end
+
+ context 'when resolve_outdated_diff_discussions is set' do
+ before do
+ discussion
+
+ subject.project.update!(resolve_outdated_diff_discussions: true)
+ end
+
+ it 'calls MergeRequests::ResolvedDiscussionNotificationService' do
+ expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService)
+ .to receive(:execute).with(subject)
+
+ subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
+ new_diff_refs: commit.diff_refs,
+ current_user: subject.author)
+ end
end
end
diff --git a/spec/services/discussions/update_diff_position_service_spec.rb b/spec/services/discussions/update_diff_position_service_spec.rb
index c239494298b..82b156f5ebe 100644
--- a/spec/services/discussions/update_diff_position_service_spec.rb
+++ b/spec/services/discussions/update_diff_position_service_spec.rb
@@ -150,21 +150,7 @@ describe Discussions::UpdateDiffPositionService do
)
end
- context "when the diff line is the same" do
- let(:line) { 16 }
-
- it "updates the position" do
- subject.execute(discussion)
-
- expect(discussion.original_position).to eq(old_position)
- expect(discussion.position).not_to eq(old_position)
- expect(discussion.position.new_line).to eq(22)
- end
- end
-
- context "when the diff line has changed" do
- let(:line) { 9 }
-
+ shared_examples 'outdated diff note' do
it "doesn't update the position" do
subject.execute(discussion)
@@ -189,5 +175,51 @@ describe Discussions::UpdateDiffPositionService do
subject.execute(discussion)
end
end
+
+ context "when the diff line is the same" do
+ let(:line) { 16 }
+
+ it "updates the position" do
+ subject.execute(discussion)
+
+ expect(discussion.original_position).to eq(old_position)
+ expect(discussion.position).not_to eq(old_position)
+ expect(discussion.position.new_line).to eq(22)
+ end
+
+ context 'when the resolve_outdated_diff_discussions setting is set' do
+ before do
+ project.update!(resolve_outdated_diff_discussions: true)
+ end
+
+ it 'does not resolve the discussion' do
+ subject.execute(discussion)
+
+ expect(discussion).not_to be_resolved
+ expect(discussion).not_to be_resolved_by_push
+ end
+ end
+ end
+
+ context "when the diff line has changed" do
+ let(:line) { 9 }
+
+ include_examples 'outdated diff note'
+
+ context 'when the resolve_outdated_diff_discussions setting is set' do
+ before do
+ project.update!(resolve_outdated_diff_discussions: true)
+ end
+
+ it 'sets resolves the discussion and sets resolved_by_push' do
+ subject.execute(discussion)
+
+ expect(discussion).to be_resolved
+ expect(discussion).to be_resolved_by_push
+ end
+
+ include_examples 'outdated diff note'
+ end
+ end
end
end
View Lorry Controller Status