From ade4dec27ba2413ccf774b54db535901d5adb918 Mon Sep 17 00:00:00 2001 From: Vincent Bonmalais Date: Tue, 16 Oct 2012 14:49:31 +1100 Subject: [PATCH 1/7] Group comments from merge requests. Comments on diff are grouped in the same fashion as github. A link has been added to go directly to the diff line. --- app/assets/stylesheets/sections/notes.scss | 9 ++ app/views/merge_requests/_show.html.haml | 4 +- app/views/merge_requests/show.js.haml | 2 +- app/views/notes/_discussion.html.haml | 7 ++ app/views/notes/_notes.html.haml | 9 +- app/views/notes/_per_line_note_link.html.haml | 2 +- features/project/merge_requests.feature | 8 ++ .../steps/project/project_merge_requests.rb | 104 ++++++++++++------ features/steps/shared/paths.rb | 5 + spec/factories.rb | 14 +++ 10 files changed, 122 insertions(+), 42 deletions(-) create mode 100644 app/views/notes/_discussion.html.haml diff --git a/app/assets/stylesheets/sections/notes.scss b/app/assets/stylesheets/sections/notes.scss index d24d070df1e..b03f4d45c87 100644 --- a/app/assets/stylesheets/sections/notes.scss +++ b/app/assets/stylesheets/sections/notes.scss @@ -8,6 +8,14 @@ list-style:none; margin:0px; padding:0px; + + .group_diff_note { + border: 1px solid #eee; + margin-top: 10px; + margin-left: 10px; + padding: 15px; + background-color: #fdfdfd; + } } .issue_notes, @@ -73,6 +81,7 @@ #notes-list.reversed .note, #new-notes-list.reversed .note { border-top: 1px solid #eee; + } /* mark vote notes */ diff --git a/app/views/merge_requests/_show.html.haml b/app/views/merge_requests/_show.html.haml index f1d0c8aaafb..d12431da70b 100644 --- a/app/views/merge_requests/_show.html.haml +++ b/app/views/merge_requests/_show.html.haml @@ -7,11 +7,11 @@ - if @commits.present? %ul.nav.nav-tabs.mr_nav_tabs %li - = link_to "#notes", "data-url" => project_merge_request_path(@project, @merge_request), class: "merge-notes-tab tab" do + = link_to "#notes", title: "Comments", "data-url" => project_merge_request_path(@project, @merge_request), class: "merge-notes-tab tab" do %i.icon-comment Comments %li - = link_to "#diffs", "data-url" => diffs_project_merge_request_path(@project, @merge_request), class: "merge-diffs-tab tab" do + = link_to "#diffs", title: "Diff", "data-url" => diffs_project_merge_request_path(@project, @merge_request), class: "merge-diffs-tab tab" do %i.icon-list-alt Diff diff --git a/app/views/merge_requests/show.js.haml b/app/views/merge_requests/show.js.haml index f44ccbb5127..e624ce51d78 100644 --- a/app/views/merge_requests/show.js.haml +++ b/app/views/merge_requests/show.js.haml @@ -1,2 +1,2 @@ :plain - $(".merge-request-notes").html("#{escape_javascript(render notes/notes_with_form", tid: @merge_request.id, tt: "merge_request")}"); + $(".merge-request-notes").html("#{escape_javascript(render("notes/notes_with_form", tid: @merge_request.id, tt: "merge_request"))}"); diff --git a/app/views/notes/_discussion.html.haml b/app/views/notes/_discussion.html.haml new file mode 100644 index 00000000000..5e73ae2ad8b --- /dev/null +++ b/app/views/notes/_discussion.html.haml @@ -0,0 +1,7 @@ +.group_diff_note + .header + = link_to diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.line_code) do + = "#{note.author.name} started a discussion on this merge request (on line #{note.line_code.split('_')[2]}):" + .body + - @notes.select {|a_note| a_note.line_code == note.line_code}.each do |a_note| + = render "note", note: a_note diff --git a/app/views/notes/_notes.html.haml b/app/views/notes/_notes.html.haml index adb5dfcbf18..e3bfcc87e1e 100644 --- a/app/views/notes/_notes.html.haml +++ b/app/views/notes/_notes.html.haml @@ -1,4 +1,9 @@ +- discussions = [] - @notes.each do |note| - next unless note.author - = render "note", note: note - + - if note.line_code.nil? + = render "note", note: note + - else + - next if discussions.include?(note.line_code) + - discussions << note.line_code + = render "discussion", note: note diff --git a/app/views/notes/_per_line_note_link.html.haml b/app/views/notes/_per_line_note_link.html.haml index 72b59a596a3..8d0f9beb964 100644 --- a/app/views/notes/_per_line_note_link.html.haml +++ b/app/views/notes/_per_line_note_link.html.haml @@ -1 +1 @@ -= link_to "", "#", class: "line_note_link", data: { line_code: line_code }, title: "Add note for this line" += link_to "", "#", id: "line-note-#{line_code}", class: "line_note_link", data: { line_code: line_code }, title: "Add note on line #{line_code[/[0-9]+\z/]}" diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 80f00986466..d8943fa8ada 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -40,3 +40,11 @@ Feature: Project Merge Requests And I leave a comment like "XML attached" Then I should see comment "XML attached" + @javascript @feat-1 + Scenario: I comment merge request diff + Given project "Shop" have "Bug NS-05" open merge request with diffs inside + And I visit merge request page "Bug NS-05" + And I switch to the diff tab + And I leave a comment like "Line is wrong" on line 182 of the first file + And I switch to the comments tab + Then I should see a discussion has started on line 182 diff --git a/features/steps/project/project_merge_requests.rb b/features/steps/project/project_merge_requests.rb index 80e83906c72..478e0144654 100644 --- a/features/steps/project/project_merge_requests.rb +++ b/features/steps/project/project_merge_requests.rb @@ -4,77 +4,109 @@ class ProjectMergeRequests < Spinach::FeatureSteps include SharedNote include SharedPaths - Then 'I should see "Bug NS-04" in merge requests' do - page.should have_content "Bug NS-04" + Given 'I click link "New Merge Request"' do + click_link "New Merge Request" end - And 'I should not see "Feature NS-03" in merge requests' do - page.should_not have_content "Feature NS-03" + Given 'I click link "Bug NS-04"' do + click_link "Bug NS-04" + end + + Given 'I click link "All"' do + click_link "All" end Given 'I click link "Closed"' do click_link "Closed" end - Then 'I should see "Feature NS-03" in merge requests' do - page.should have_content "Feature NS-03" + Then 'I should see merge request "Wiki Feature"' do + page.should have_content "Wiki Feature" end - And 'I should not see "Bug NS-04" in merge requests' do - page.should_not have_content "Bug NS-04" + Then 'I should see closed merge request "Bug NS-04"' do + mr = MergeRequest.find_by_title("Bug NS-04") + mr.closed.should be_true + page.should have_content "Closed by" end - Given 'I click link "All"' do - click_link "All" + Then 'I should see merge request "Bug NS-04"' do + page.should have_content "Bug NS-04" end - Given 'I click link "Bug NS-04"' do - click_link "Bug NS-04" + Then 'I should see "Bug NS-04" in merge requests' do + page.should have_content "Bug NS-04" end - Then 'I should see merge request "Bug NS-04"' do - page.should have_content "Bug NS-04" + Then 'I should see "Feature NS-03" in merge requests' do + page.should have_content "Feature NS-03" end - And 'I click link "Close"' do - click_link "Close" + And 'I should not see "Feature NS-03" in merge requests' do + page.should_not have_content "Feature NS-03" end - Then 'I should see closed merge request "Bug NS-04"' do - mr = MergeRequest.find_by_title("Bug NS-04") - mr.closed.should be_true - page.should have_content "Closed by" + + And 'I should not see "Bug NS-04" in merge requests' do + page.should_not have_content "Bug NS-04" end - Given 'I click link "New Merge Request"' do - click_link "New Merge Request" + And 'I click link "Close"' do + click_link "Close" end And 'I submit new merge request "Wiki Feature"' do - fill_in "merge_request_title", :with => "Wiki Feature" - select "master", :from => "merge_request_source_branch" - select "stable", :from => "merge_request_target_branch" + fill_in "merge_request_title", with: "Wiki Feature" + select "master", from: "merge_request_source_branch" + select "stable", from: "merge_request_target_branch" click_button "Save" end - Then 'I should see merge request "Wiki Feature"' do - page.should have_content "Wiki Feature" - end - And 'project "Shop" have "Bug NS-04" open merge request' do project = Project.find_by_name("Shop") Factory.create(:merge_request, - :title => "Bug NS-04", - :project => project, - :author => project.users.first) + title: "Bug NS-04", + project: project, + author: project.users.first) + end + + And 'project "Shop" have "Bug NS-05" open merge request with diffs inside' do + project = Project.find_by_name("Shop") + Factory.create(:merge_request_with_diffs, + title: "Bug NS-05", + project: project, + author: project.users.first) end And 'project "Shop" have "Feature NS-03" closed merge request' do project = Project.find_by_name("Shop") Factory.create(:merge_request, - :title => "Feature NS-03", - :project => project, - :author => project.users.first, - :closed => true) + title: "Feature NS-03", + project: project, + author: project.users.first, + closed: true) + end + + And 'I switch to the diff tab' do + mr = MergeRequest.find_by_title("Bug NS-05") + visit diffs_project_merge_request_path(mr.project, mr) + end + + And 'I switch to the comments tab' do + mr = MergeRequest.find_by_title("Bug NS-05") + visit project_merge_request_path(mr.project, mr) + end + + And 'I leave a comment like "Line is wrong" on line 182 of the first file' do + within(:xpath, "//div[@class='diff_file'][1]") do + click_link "Add note on line 182" + fill_in "note_note", with: "Line is wrong" + click_button "Add note" + end + end + + Then 'I should see a discussion has started on line 182' do + page.should have_content "#{current_user.name} started a discussion on this merge request (on line 182):" + page.should have_content "Line is wrong" end end diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index eda5ab94116..d401e916175 100644 --- a/features/steps/shared/paths.rb +++ b/features/steps/shared/paths.rb @@ -192,6 +192,11 @@ module SharedPaths visit project_merge_request_path(mr.project, mr) end + Given 'I visit merge request page "Bug NS-05"' do + mr = MergeRequest.find_by_title("Bug NS-05") + visit project_merge_request_path(mr.project, mr) + end + And 'I visit project "Shop" merge requests page' do visit project_merge_requests_path(Project.find_by_name("Shop")) end diff --git a/spec/factories.rb b/spec/factories.rb index 0258f8920e9..72745c665c4 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -82,6 +82,20 @@ FactoryGirl.define do end factory :closed_merge_request, traits: [:closed] + + factory :merge_request_with_diffs do + st_commits do + repo = Grit::Repo.new(Rails.root.join('tmp', 'repositories', 'gitlabhq')) + [Commit.new(repo.commit('bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a'))] + end + st_diffs do + diffs = [] + st_commits.each do |commit| + diffs += commit.diffs + end + diffs + end + end end factory :note do -- GitLab From 67adf2f6d350a0be4f89d330d19579de4722ca13 Mon Sep 17 00:00:00 2001 From: Vincent Bonmalais Date: Wed, 17 Oct 2012 14:47:59 +1100 Subject: [PATCH 2/7] Refactor changes in an helper. Commit notes and Merge request notes are now showns in the same way. --- app/assets/stylesheets/sections/notes.scss | 21 +++++-- app/helpers/discussion_helper.rb | 62 +++++++++++++++++++ app/helpers/notes_helper.rb | 14 +++++ app/views/notes/_discussion.html.haml | 24 +++++-- app/views/notes/_note.html.haml | 6 -- app/views/notes/_notes.html.haml | 9 +-- features/project/merge_requests.feature | 2 +- .../steps/project/project_merge_requests.rb | 6 +- 8 files changed, 119 insertions(+), 25 deletions(-) create mode 100644 app/helpers/discussion_helper.rb diff --git a/app/assets/stylesheets/sections/notes.scss b/app/assets/stylesheets/sections/notes.scss index b03f4d45c87..3f049092420 100644 --- a/app/assets/stylesheets/sections/notes.scss +++ b/app/assets/stylesheets/sections/notes.scss @@ -9,12 +9,21 @@ margin:0px; padding:0px; - .group_diff_note { - border: 1px solid #eee; - margin-top: 10px; - margin-left: 10px; - padding: 15px; - background-color: #fdfdfd; + .note_discussion { + @extend .borders; + @extend .prepend-top-20; + @extend .append-bottom-20; + border-width:1px; + @include solid_shade; + + .header { + padding: 15px; + background-color: whiteSmoke; + border-bottom: 1px solid #EEE; + } + .body { + padding: 15px; + } } } diff --git a/app/helpers/discussion_helper.rb b/app/helpers/discussion_helper.rb new file mode 100644 index 00000000000..ad71467ca2c --- /dev/null +++ b/app/helpers/discussion_helper.rb @@ -0,0 +1,62 @@ +module DiscussionHelper + def part_of_discussion?(note) + case note.noteable_type + when 'MergeRequest' + note.line_code.present? + when 'Commit' + true + else + false + end + end + + def discussion_title(note) + case note.noteable_type + when 'MergeRequest' + "#{note.author.name} started a discussion on this merge request (on line #{note.line_code.split('_')[2]}):" + when 'Commit' + if note.line_code.present? + "#{note.author.name} started a discussion on commit #{link_to_commit_diff_line_note(note)}:" + else + "#{note.author.name} started a discussion on commit #{note.noteable_id}:" + end + else + "#{note.author.name} started a discussion:" + end + end + + def render_discussion(note) + unless discussions_for(note.noteable_type).include?(discussion_id_for(note)) + discussions_for(note.noteable_type).push(discussion_id_for(note)) + + render 'discussion', note: note + end + end + + def has_rendered?(note) + discussions_for(note.noteable_type).include?(discussion_id_for(note)) + end + + def discussion_notes(note) + @notes.select do |other_note| + discussion_id_for(note) == discussion_id_for(other_note) + end + end + + def discussion_id_for(note) + [note.line_code, note.noteable_id, note.noteable_type] + end + + def discussions_for(type) + @discussions ||= { merge_requests: [], commits: [] } + + case type + when 'MergeRequest' + @discussions[:merge_requests] + when 'Commit' + @discussions[:commits] + else + [] + end + end +end diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index ffcc7acc8da..75d7815454e 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -1,4 +1,6 @@ module NotesHelper + include DiscussionHelper + def loading_more_notes? params[:loading_more].present? end @@ -21,4 +23,16 @@ module NotesHelper link_to "#{link_file}:L#{link_line}", project_commit_path(@project, commit, anchor: note.line_code) end + + def link_to_merge_request_diff_line_note(note) + return unless note.line_note? + + mr = note.target + diff_index, diff_old_line, diff_new_line = note.line_code.split('_') + + link_file = mr.diffs[diff_index.to_i].b_path + link_line = diff_new_line + + link_to "#{link_file}:L#{link_line}", diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.line_code) + end end diff --git a/app/views/notes/_discussion.html.haml b/app/views/notes/_discussion.html.haml index 5e73ae2ad8b..326db668bd1 100644 --- a/app/views/notes/_discussion.html.haml +++ b/app/views/notes/_discussion.html.haml @@ -1,7 +1,21 @@ -.group_diff_note +.note_discussion .header - = link_to diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.line_code) do - = "#{note.author.name} started a discussion on this merge request (on line #{note.line_code.split('_')[2]}):" + %strong= note.author_name + - case note.noteable_type + - when 'MergeRequest' + %cite.cgray + started a discussion on this merge request diff + = link_to_merge_request_diff_line_note(note) + \: + - when 'Commit' + %cite.cgray + started a discussion on commit + #{link_to note.target.short_id, project_commit_path(@project, note.target)} + = link_to_commit_diff_line_note(note) if note.line_note? + \: + - else + %cite.cgray + started a discussion: .body - - @notes.select {|a_note| a_note.line_code == note.line_code}.each do |a_note| - = render "note", note: a_note + - discussion_notes(note).each do |a_note| + = render 'note', note: a_note diff --git a/app/views/notes/_note.html.haml b/app/views/notes/_note.html.haml index 70baa212d10..57946163b56 100644 --- a/app/views/notes/_note.html.haml +++ b/app/views/notes/_note.html.haml @@ -7,12 +7,6 @@ = time_ago_in_words(note.updated_at) ago - - unless note_for_main_target?(note) - - if note.for_commit? - %span.cgray - on #{link_to note.noteable.short_id, project_commit_path(@project, note.noteable)} - = link_to_commit_diff_line_note(note) if note.for_diff_line? - -# only show vote if it's a note for the main target - if note_for_main_target?(note) - if note.upvote? diff --git a/app/views/notes/_notes.html.haml b/app/views/notes/_notes.html.haml index e3bfcc87e1e..b6b1c167d20 100644 --- a/app/views/notes/_notes.html.haml +++ b/app/views/notes/_notes.html.haml @@ -1,9 +1,6 @@ -- discussions = [] - @notes.each do |note| - next unless note.author - - if note.line_code.nil? - = render "note", note: note + - if part_of_discussion?(note) + = render_discussion(note) - else - - next if discussions.include?(note.line_code) - - discussions << note.line_code - = render "discussion", note: note + = render 'note', note: note diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index d8943fa8ada..0cbeda22e84 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -40,7 +40,7 @@ Feature: Project Merge Requests And I leave a comment like "XML attached" Then I should see comment "XML attached" - @javascript @feat-1 + @javascript Scenario: I comment merge request diff Given project "Shop" have "Bug NS-05" open merge request with diffs inside And I visit merge request page "Bug NS-05" diff --git a/features/steps/project/project_merge_requests.rb b/features/steps/project/project_merge_requests.rb index 478e0144654..b4d576bea1c 100644 --- a/features/steps/project/project_merge_requests.rb +++ b/features/steps/project/project_merge_requests.rb @@ -106,7 +106,11 @@ class ProjectMergeRequests < Spinach::FeatureSteps end Then 'I should see a discussion has started on line 182' do - page.should have_content "#{current_user.name} started a discussion on this merge request (on line 182):" + mr = MergeRequest.find_by_title("Bug NS-05") + first_commit = mr.st_commits.first + first_diff = mr.st_diffs.first + page.should have_content "#{current_user.name} started a discussion on this merge request diff" + page.should have_content "#{first_diff.b_path}:L182" page.should have_content "Line is wrong" end end -- GitLab From 02ef54cf520f889d7af34bae9b4b8023ae3d7e79 Mon Sep 17 00:00:00 2001 From: Vincent Bonmalais Date: Wed, 17 Oct 2012 21:09:51 +1100 Subject: [PATCH 3/7] Add some sugar! Diff are now included in the merge request comments. --- app/assets/stylesheets/sections/notes.scss | 36 +++++++++++---- app/controllers/merge_requests_controller.rb | 2 +- app/helpers/discussion_helper.rb | 12 +++-- app/helpers/notes_helper.rb | 22 +++------ app/models/note.rb | 16 +++++++ app/views/commits/_text_file.html.haml | 2 +- app/views/notes/_discussion.html.haml | 48 ++++++++++++-------- 7 files changed, 87 insertions(+), 51 deletions(-) diff --git a/app/assets/stylesheets/sections/notes.scss b/app/assets/stylesheets/sections/notes.scss index 3f049092420..1e611441f53 100644 --- a/app/assets/stylesheets/sections/notes.scss +++ b/app/assets/stylesheets/sections/notes.scss @@ -10,19 +10,37 @@ padding:0px; .note_discussion { - @extend .borders; - @extend .prepend-top-20; - @extend .append-bottom-20; - border-width:1px; - @include solid_shade; - .header { - padding: 15px; + padding: 5px; + padding-bottom: 15px; + + .text { + padding-top: 9px; + } + } + + .body, .diff_file { + @extend .borders; + @extend .prepend-top-20; + @extend .append-bottom-20; + border-width:1px; background-color: whiteSmoke; - border-bottom: 1px solid #EEE; + margin-left: 15px; + + .note { + border-bottom: 0px; + padding: 0px; + } } + .body { - padding: 15px; + .note { + border-bottom: 1px solid #ddd; + padding: 15px; + } + .note:nth-child(odd) { + background-color: #fafafa; + } } } } diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 8e180c9baae..39680ef96ca 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -36,7 +36,7 @@ class MergeRequestsController < ProjectResourceController @diffs = @merge_request.diffs @commit = @merge_request.last_commit - @comments_allowed = true + @comments_allowed = @reply_allowed = true @line_notes = @merge_request.notes.where("line_code is not null") end diff --git a/app/helpers/discussion_helper.rb b/app/helpers/discussion_helper.rb index ad71467ca2c..73aa56b8310 100644 --- a/app/helpers/discussion_helper.rb +++ b/app/helpers/discussion_helper.rb @@ -29,14 +29,16 @@ module DiscussionHelper unless discussions_for(note.noteable_type).include?(discussion_id_for(note)) discussions_for(note.noteable_type).push(discussion_id_for(note)) - render 'discussion', note: note + if note.line_note? + @reply_allowed = true + @line_notes = discussion_notes(note) + render 'discussion', note: note, diff: note.diff + else + render 'discussion', note: note + end end end - def has_rendered?(note) - discussions_for(note.noteable_type).include?(discussion_id_for(note)) - end - def discussion_notes(note) @notes.select do |other_note| discussion_id_for(note) == discussion_id_for(other_note) diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 75d7815454e..ef7ac7d4475 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -15,24 +15,14 @@ module NotesHelper end def link_to_commit_diff_line_note(note) - commit = note.noteable - diff_index, diff_old_line, diff_new_line = note.line_code.split('_') - - link_file = commit.diffs[diff_index.to_i].new_path - link_line = diff_new_line - - link_to "#{link_file}:L#{link_line}", project_commit_path(@project, commit, anchor: note.line_code) + if note.line_note? + link_to "#{note.diff.new_path}:L#{note.diff_new_line}", project_commit_path(@project, note.noteable, anchor: note.line_code) + end end def link_to_merge_request_diff_line_note(note) - return unless note.line_note? - - mr = note.target - diff_index, diff_old_line, diff_new_line = note.line_code.split('_') - - link_file = mr.diffs[diff_index.to_i].b_path - link_line = diff_new_line - - link_to "#{link_file}:L#{link_line}", diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.line_code) + if note.line_note? + link_to "#{note.diff.b_path}:L#{note.diff_new_line}", diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.line_code) + end end end diff --git a/app/models/note.rb b/app/models/note.rb index e2f4a89d7d6..6bc84468f6b 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -106,6 +106,22 @@ class Note < ActiveRecord::Base def downvote? note.start_with?('-1') || note.start_with?(':-1:') end + + def diff_index + line_code.split('_')[0].to_i + end + + def diff_old_line + line_code.split('_')[1].to_i + end + + def diff_new_line + line_code.split('_')[2].to_i + end + + def diff + target.diffs[diff_index] + end end # == Schema Information diff --git a/app/views/commits/_text_file.html.haml b/app/views/commits/_text_file.html.haml index 02117386d71..5e7886922b4 100644 --- a/app/views/commits/_text_file.html.haml +++ b/app/views/commits/_text_file.html.haml @@ -17,7 +17,7 @@ %td.new_line= link_to raw(type == "old" ? " " : line_new) , "##{line_code}", id: line_code %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw "#{line}  " - - if @comments_allowed + - if @reply_allowed - comments = @line_notes.select { |n| n.line_code == line_code }.sort_by(&:created_at) - unless comments.empty? = render "notes/per_line_notes_with_reply", notes: comments diff --git a/app/views/notes/_discussion.html.haml b/app/views/notes/_discussion.html.haml index 326db668bd1..991374381ac 100644 --- a/app/views/notes/_discussion.html.haml +++ b/app/views/notes/_discussion.html.haml @@ -1,21 +1,31 @@ .note_discussion .header - %strong= note.author_name - - case note.noteable_type - - when 'MergeRequest' - %cite.cgray - started a discussion on this merge request diff - = link_to_merge_request_diff_line_note(note) - \: - - when 'Commit' - %cite.cgray - started a discussion on commit - #{link_to note.target.short_id, project_commit_path(@project, note.target)} - = link_to_commit_diff_line_note(note) if note.line_note? - \: - - else - %cite.cgray - started a discussion: - .body - - discussion_notes(note).each do |a_note| - = render 'note', note: a_note + = image_tag gravatar_icon(note.author.email), class: "avatar s32" + .text + %strong= note.author_name + - case note.noteable_type + - when 'MergeRequest' + %cite.cgray started a discussion on this merge request diff + = link_to_merge_request_diff_line_note(note) + - when 'Commit' + %cite.cgray started a discussion on commit + #{link_to note.target.short_id, project_commit_path(@project, note.target)} + = link_to_commit_diff_line_note(note) if note.line_note? + - else + %cite.cgray started a discussion + - if note.line_code? + .diff_file + .diff_file_header + - if diff.deleted_file + %i.icon-file + %span{id: "#{diff.a_path}"}= diff.a_path + - else + %i.icon-file + %span{id: "#{diff.b_path}"}= diff.b_path + %br/ + .diff_file_content + = render "commits/text_file", diff: note.diff, index: note.diff_index + - else + .body + - discussion_notes(note).each do |a_note| + = render 'note', note: a_note -- GitLab From 048028c29daced96d345c0b8e4a5ca6d150627dd Mon Sep 17 00:00:00 2001 From: Vincent Bonmalais Date: Thu, 18 Oct 2012 11:41:12 +1100 Subject: [PATCH 4/7] Fix reply button on MR page. Also update notes to display only discussions on the merge_request page. --- app/assets/javascripts/merge_requests.js | 12 +++++++++++- app/assets/javascripts/notes.js | 12 +++++++++++- app/controllers/commit_controller.rb | 2 +- app/controllers/notes_controller.rb | 2 ++ app/helpers/discussion_helper.rb | 18 ++++++++++-------- app/views/merge_requests/diffs.html.haml | 5 ----- app/views/notes/_create_per_line_note.js.haml | 2 +- .../notes/_per_line_notes_with_reply.html.haml | 2 +- .../notes/_per_line_reply_button.html.haml | 8 +++++++- app/views/notes/index.js.haml | 4 ++++ 10 files changed, 48 insertions(+), 19 deletions(-) diff --git a/app/assets/javascripts/merge_requests.js b/app/assets/javascripts/merge_requests.js index 0ab6f6e22a1..43597108177 100644 --- a/app/assets/javascripts/merge_requests.js +++ b/app/assets/javascripts/merge_requests.js @@ -15,7 +15,17 @@ var MergeRequest = { self.showAllCommits(); }); - $(".line_note_link, .line_note_reply_link").live("click", function(e) { + $(".line_note_reply_link").live("click", function(e) { + var form = $(".per_line_form"); + $(this).parent().parent().after(form); + form.find("#note_line_code").val($(this).attr("line_code")); + form.find("#note_noteable_type").val($(this).attr("noteable_type")); + form.find("#note_noteable_id").val($(this).attr("noteable_id")); + form.show(); + return false; + }); + + $(".line_note_link").live("click", function(e) { var form = $(".per_line_form"); $(this).parent().parent().after(form); form.find("#note_line_code").val($(this).attr("line_code")); diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 558643d504d..193774fcbff 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -259,7 +259,7 @@ var PerLineNotes = { * Shows the note form below the line. * Sets some hidden fields in the form. */ - $(".diff_file_content").on("click", ".line_note_link, .line_note_reply_link", function(e) { + $(".diff_file_content").on("click", ".line_note_link", function(e) { var form = $(".per_line_form"); $(this).closest("tr").after(form); form.find("#note_line_code").val($(this).data("lineCode")); @@ -267,6 +267,16 @@ var PerLineNotes = { return false; }); + $(".diff_file_content").on("click", ".line_note_reply_link", function(e) { + var form = $(".per_line_form"); + $(this).closest("tr").after(form); + form.find("#note_line_code").val($(this).data("lineCode")); + form.find("#note_noteable_type").val($(this).data("noteableType")); + form.find("#note_noteable_id").val($(this).data("noteableId")); + form.show(); + return false; + }); + disableButtonIfEmptyField(".line-note-text", ".submit_inline_note"); /** diff --git a/app/controllers/commit_controller.rb b/app/controllers/commit_controller.rb index 97998352255..214694be64c 100644 --- a/app/controllers/commit_controller.rb +++ b/app/controllers/commit_controller.rb @@ -17,7 +17,7 @@ class CommitController < ProjectResourceController @note = result[:note] @line_notes = result[:line_notes] @notes_count = result[:notes_count] - @comments_allowed = true + @comments_allowed = @reply_allowed = true respond_to do |format| format.html do diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index d794f368f57..89e12177cfa 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -10,6 +10,7 @@ class NotesController < ProjectResourceController if params[:target_type] == "merge_request" @mixed_targets = true @main_target_type = params[:target_type].camelize + @show_discussions = true end respond_with(@notes) @@ -42,5 +43,6 @@ class NotesController < ProjectResourceController def notes @notes = Notes::LoadContext.new(project, current_user, params).execute + @has_diff = @notes.any? { |note| note.line_note? } end end diff --git a/app/helpers/discussion_helper.rb b/app/helpers/discussion_helper.rb index 73aa56b8310..61c189d5ad0 100644 --- a/app/helpers/discussion_helper.rb +++ b/app/helpers/discussion_helper.rb @@ -1,13 +1,15 @@ module DiscussionHelper def part_of_discussion?(note) - case note.noteable_type - when 'MergeRequest' - note.line_code.present? - when 'Commit' - true - else - false - end + part_of_discussion = case note.noteable_type + when 'MergeRequest' + note.line_code.present? + when 'Commit' + true + else + false + end + + part_of_discussion && @show_discussions end def discussion_title(note) diff --git a/app/views/merge_requests/diffs.html.haml b/app/views/merge_requests/diffs.html.haml index a755491c42e..2a5b8b1441e 100644 --- a/app/views/merge_requests/diffs.html.haml +++ b/app/views/merge_requests/diffs.html.haml @@ -1,6 +1 @@ = render "show" - -:javascript - $(function(){ - PerLineNotes.init(); - }); diff --git a/app/views/notes/_create_per_line_note.js.haml b/app/views/notes/_create_per_line_note.js.haml index 180960e38e4..eea9eb38be4 100644 --- a/app/views/notes/_create_per_line_note.js.haml +++ b/app/views/notes/_create_per_line_note.js.haml @@ -11,7 +11,7 @@ // find the commented line ... var trEl = $(".#{note.line_code}").parent(); // ... and insert the note and the reply button after it - trEl.after("#{escape_javascript(render "notes/per_line_reply_button", line_code: note.line_code)}"); + trEl.after("#{escape_javascript(render "notes/per_line_reply_button", note: note)}"); trEl.after("#{escape_javascript(render "notes/per_line_note", note: note)}"); } else { // instert new note before reply button diff --git a/app/views/notes/_per_line_notes_with_reply.html.haml b/app/views/notes/_per_line_notes_with_reply.html.haml index 1bcfc41fe20..ebe0c5c6c47 100644 --- a/app/views/notes/_per_line_notes_with_reply.html.haml +++ b/app/views/notes/_per_line_notes_with_reply.html.haml @@ -1,3 +1,3 @@ - notes.each do |note| = render "notes/per_line_note", note: note -= render "notes/per_line_reply_button", line_code: notes.first.line_code += render "notes/per_line_reply_button", note: notes.first diff --git a/app/views/notes/_per_line_reply_button.html.haml b/app/views/notes/_per_line_reply_button.html.haml index 42c737c75ae..ac3884a328c 100644 --- a/app/views/notes/_per_line_reply_button.html.haml +++ b/app/views/notes/_per_line_reply_button.html.haml @@ -1,4 +1,10 @@ %tr.line_notes_row.reply %td{colspan: 3} %i.icon-comment - = link_to "Reply", "#", class: "line_note_reply_link", data: { line_code: line_code }, title: "Add note for this line" + = link_to "Reply", + "#", + class: "line_note_reply_link", + data: { line_code: note.line_code, + noteable_type: note.noteable_type, + noteable_id: note.noteable_id }, + title: "Add note for this line" diff --git a/app/views/notes/index.js.haml b/app/views/notes/index.js.haml index 3814dbd46a2..99da619c649 100644 --- a/app/views/notes/index.js.haml +++ b/app/views/notes/index.js.haml @@ -15,3 +15,7 @@ - if loading_more_notes? :plain NoteList.finishedLoadingMore(); + +- if @has_diff + :plain + PerLineNotes.init(); -- GitLab From 2a8f2d5de5c5db3860769b500b0522b82ea1af8b Mon Sep 17 00:00:00 2001 From: Vincent Bonmalais Date: Thu, 18 Oct 2012 22:30:49 +1100 Subject: [PATCH 5/7] Add tests for different type of discussions. Also, refactor notes to be able to have the same behavior on 2 different classes (.line_note_link and .line_note_reply_link) --- app/assets/javascripts/merge_requests.js | 10 +---- app/assets/javascripts/notes.js | 10 +---- app/controllers/commit_controller.rb | 2 + app/controllers/merge_requests_controller.rb | 2 + app/controllers/notes_controller.rb | 7 ++- app/views/notes/_per_line_note_link.html.haml | 2 +- features/project/merge_requests.feature | 28 +++++++++--- .../steps/project/project_merge_requests.rb | 45 ++++++++++++++++--- 8 files changed, 75 insertions(+), 31 deletions(-) diff --git a/app/assets/javascripts/merge_requests.js b/app/assets/javascripts/merge_requests.js index 43597108177..561cff66319 100644 --- a/app/assets/javascripts/merge_requests.js +++ b/app/assets/javascripts/merge_requests.js @@ -15,7 +15,7 @@ var MergeRequest = { self.showAllCommits(); }); - $(".line_note_reply_link").live("click", function(e) { + $(".line_note_link, .line_note_reply_link").live("click", function(e) { var form = $(".per_line_form"); $(this).parent().parent().after(form); form.find("#note_line_code").val($(this).attr("line_code")); @@ -24,14 +24,6 @@ var MergeRequest = { form.show(); return false; }); - - $(".line_note_link").live("click", function(e) { - var form = $(".per_line_form"); - $(this).parent().parent().after(form); - form.find("#note_line_code").val($(this).attr("line_code")); - form.show(); - return false; - }); }, initMergeWidget: diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 193774fcbff..b561d8fdec3 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -259,15 +259,7 @@ var PerLineNotes = { * Shows the note form below the line. * Sets some hidden fields in the form. */ - $(".diff_file_content").on("click", ".line_note_link", function(e) { - var form = $(".per_line_form"); - $(this).closest("tr").after(form); - form.find("#note_line_code").val($(this).data("lineCode")); - form.show(); - return false; - }); - - $(".diff_file_content").on("click", ".line_note_reply_link", function(e) { + $(".diff_file_content").on("click", ".line_note_link, .line_note_reply_link", function(e) { var form = $(".per_line_form"); $(this).closest("tr").after(form); form.find("#note_line_code").val($(this).data("lineCode")); diff --git a/app/controllers/commit_controller.rb b/app/controllers/commit_controller.rb index 214694be64c..89efaf49d81 100644 --- a/app/controllers/commit_controller.rb +++ b/app/controllers/commit_controller.rb @@ -18,6 +18,8 @@ class CommitController < ProjectResourceController @line_notes = result[:line_notes] @notes_count = result[:notes_count] @comments_allowed = @reply_allowed = true + @comments_target = { noteable_type: 'Commit', + noteable_id: @commit.id } respond_to do |format| format.html do diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 39680ef96ca..efa04e67a79 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -37,6 +37,8 @@ class MergeRequestsController < ProjectResourceController @commit = @merge_request.last_commit @comments_allowed = @reply_allowed = true + @comments_target = { noteable_type: 'MergeRequest', + noteable_id: @merge_request.id } @line_notes = @merge_request.notes.where("line_code is not null") end diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 89e12177cfa..2dc0abf546e 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -7,10 +7,14 @@ class NotesController < ProjectResourceController def index notes + if params[:target_type] == "merge_request" - @mixed_targets = true + @mixed_targets = true @main_target_type = params[:target_type].camelize @show_discussions = true + @has_diff = true + elsif params[:target_type] == "commit" + @has_diff = true end respond_with(@notes) @@ -43,6 +47,5 @@ class NotesController < ProjectResourceController def notes @notes = Notes::LoadContext.new(project, current_user, params).execute - @has_diff = @notes.any? { |note| note.line_note? } end end diff --git a/app/views/notes/_per_line_note_link.html.haml b/app/views/notes/_per_line_note_link.html.haml index 8d0f9beb964..0a4d2e76a08 100644 --- a/app/views/notes/_per_line_note_link.html.haml +++ b/app/views/notes/_per_line_note_link.html.haml @@ -1 +1 @@ -= link_to "", "#", id: "line-note-#{line_code}", class: "line_note_link", data: { line_code: line_code }, title: "Add note on line #{line_code[/[0-9]+\z/]}" += link_to "", "#", id: "line-note-#{line_code}", class: "line_note_link", data: @comments_target.merge({ line_code: line_code }), title: "Add note on line #{line_code[/[0-9]+\z/]}" diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 0cbeda22e84..5b8becbb6c9 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -35,16 +35,34 @@ Feature: Project Merge Requests Then I should see merge request "Wiki Feature" @javascript - Scenario: I comment merge request + Scenario: I comment on a merge request Given I visit merge request page "Bug NS-04" And I leave a comment like "XML attached" Then I should see comment "XML attached" @javascript - Scenario: I comment merge request diff + Scenario: I comment on a merge request diff Given project "Shop" have "Bug NS-05" open merge request with diffs inside And I visit merge request page "Bug NS-05" And I switch to the diff tab - And I leave a comment like "Line is wrong" on line 182 of the first file - And I switch to the comments tab - Then I should see a discussion has started on line 182 + And I leave a comment like "Line is wrong" on line 185 of the first file + And I switch to the merge request's comments tab + Then I should see a discussion has started on line 185 + + @javascript + Scenario: I comment on a line of a commit in merge request + Given project "Shop" have "Bug NS-05" open merge request with diffs inside + And I visit merge request page "Bug NS-05" + And I click on the first commit in the merge request + And I leave a comment like "Line is wrong" on line 185 of the first file + And I switch to the merge request's comments tab + Then I should see a discussion has started on commit bcf03b5de6c:L185 + + @javascript + Scenario: I comment on a commit in merge request + Given project "Shop" have "Bug NS-05" open merge request with diffs inside + And I visit merge request page "Bug NS-05" + And I click on the first commit in the merge request + And I leave a comment on the diff page + And I switch to the merge request's comments tab + Then I should see a discussion has started on commit bcf03b5de6c diff --git a/features/steps/project/project_merge_requests.rb b/features/steps/project/project_merge_requests.rb index b4d576bea1c..3b9464c77d6 100644 --- a/features/steps/project/project_merge_requests.rb +++ b/features/steps/project/project_merge_requests.rb @@ -92,25 +92,60 @@ class ProjectMergeRequests < Spinach::FeatureSteps visit diffs_project_merge_request_path(mr.project, mr) end - And 'I switch to the comments tab' do + And 'I switch to the merge request\'s comments tab' do mr = MergeRequest.find_by_title("Bug NS-05") visit project_merge_request_path(mr.project, mr) end - And 'I leave a comment like "Line is wrong" on line 182 of the first file' do + And 'I click on the first commit in the merge request' do + mr = MergeRequest.find_by_title("Bug NS-05") + click_link mr.st_commits.first.short_id(8) + end + + And 'I leave a comment on the diff page' do + within(:xpath, "//div[@class='note-form-holder']") do + fill_in "note_note", with: "One comment to rule them all" + click_button "Add Comment" + end + end + + And 'I leave a comment like "Line is wrong" on line 185 of the first file' do within(:xpath, "//div[@class='diff_file'][1]") do - click_link "Add note on line 182" + click_link "Add note on line 185" + end + + within(:xpath, "//div[@class='line-note-form-holder']") do fill_in "note_note", with: "Line is wrong" click_button "Add note" end end - Then 'I should see a discussion has started on line 182' do + Then 'I should see a discussion has started on line 185' do mr = MergeRequest.find_by_title("Bug NS-05") first_commit = mr.st_commits.first first_diff = mr.st_diffs.first page.should have_content "#{current_user.name} started a discussion on this merge request diff" - page.should have_content "#{first_diff.b_path}:L182" + page.should have_content "#{first_diff.b_path}:L185" page.should have_content "Line is wrong" end + + Then 'I should see a discussion has started on commit bcf03b5de6c:L185' do + mr = MergeRequest.find_by_title("Bug NS-05") + first_commit = mr.st_commits.first + first_diff = mr.st_diffs.first + page.should have_content "#{current_user.name} started a discussion on commit" + page.should have_content first_commit.short_id(8) + page.should have_content "#{first_diff.b_path}:L185" + page.should have_content "Line is wrong" + end + + Then 'I should see a discussion has started on commit bcf03b5de6c' do + mr = MergeRequest.find_by_title("Bug NS-05") + first_commit = mr.st_commits.first + first_diff = mr.st_diffs.first + page.should have_content "#{current_user.name} started a discussion on commit" + page.should have_content first_commit.short_id(8) + page.should have_content "One comment to rule them all" + page.should_not have_content "#{first_diff.b_path}:L185" + end end -- GitLab From 0f09334984a754f36286dce658d3bb188e6b5ed0 Mon Sep 17 00:00:00 2001 From: Vincent Bonmalais Date: Fri, 19 Oct 2012 15:33:12 +1100 Subject: [PATCH 6/7] Improve readability by using for_*? methods. --- app/helpers/discussion_helper.rb | 39 +++++++++------------------ app/helpers/notes_helper.rb | 8 +++--- app/models/note.rb | 14 +++++++++- app/views/notes/_discussion.html.haml | 13 +++++---- app/views/notes/_notes.html.haml | 2 +- 5 files changed, 37 insertions(+), 39 deletions(-) diff --git a/app/helpers/discussion_helper.rb b/app/helpers/discussion_helper.rb index 61c189d5ad0..2b72373a905 100644 --- a/app/helpers/discussion_helper.rb +++ b/app/helpers/discussion_helper.rb @@ -1,37 +1,25 @@ module DiscussionHelper def part_of_discussion?(note) - part_of_discussion = case note.noteable_type - when 'MergeRequest' - note.line_code.present? - when 'Commit' - true - else - false - end - - part_of_discussion && @show_discussions + note.for_commit? || note.for_merge_request_diff_line? end def discussion_title(note) - case note.noteable_type - when 'MergeRequest' - "#{note.author.name} started a discussion on this merge request (on line #{note.line_code.split('_')[2]}):" - when 'Commit' - if note.line_code.present? - "#{note.author.name} started a discussion on commit #{link_to_commit_diff_line_note(note)}:" - else - "#{note.author.name} started a discussion on commit #{note.noteable_id}:" - end + if note.for_merge_request? + "#{note.author.name} started a discussion on this merge request (on line #{note.diff_new_line}):" + elsif note.for_commit_diff_line? + "#{note.author.name} started a discussion on commit #{link_to_commit_diff_line_note(note)}:" + elsif note.for_commit? + "#{note.author.name} started a discussion on commit #{note.noteable_id}:" else "#{note.author.name} started a discussion:" end end def render_discussion(note) - unless discussions_for(note.noteable_type).include?(discussion_id_for(note)) - discussions_for(note.noteable_type).push(discussion_id_for(note)) + unless discussions_for(note).include?(discussion_id_for(note)) + discussions_for(note).push(discussion_id_for(note)) - if note.line_note? + if note.for_diff_line? @reply_allowed = true @line_notes = discussion_notes(note) render 'discussion', note: note, diff: note.diff @@ -51,13 +39,12 @@ module DiscussionHelper [note.line_code, note.noteable_id, note.noteable_type] end - def discussions_for(type) + def discussions_for(note) @discussions ||= { merge_requests: [], commits: [] } - case type - when 'MergeRequest' + if note.for_merge_request? @discussions[:merge_requests] - when 'Commit' + elsif note.for_commit? @discussions[:commits] else [] diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index ef7ac7d4475..dbcd2a918ed 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -15,14 +15,14 @@ module NotesHelper end def link_to_commit_diff_line_note(note) - if note.line_note? - link_to "#{note.diff.new_path}:L#{note.diff_new_line}", project_commit_path(@project, note.noteable, anchor: note.line_code) + if note.for_diff_line? + link_to "#{note.diff.new_path}:L#{note.diff_new_line}", project_commit_path(@project, note.noteable, anchor: note.noteable) end end def link_to_merge_request_diff_line_note(note) - if note.line_note? - link_to "#{note.diff.b_path}:L#{note.diff_new_line}", diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.line_code) + if note.for_diff_line? + link_to "#{note.diff.b_path}:L#{note.diff_new_line}", diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.noteable) end end end diff --git a/app/models/note.rb b/app/models/note.rb index 6bc84468f6b..8b7d6b443d4 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -83,6 +83,18 @@ class Note < ActiveRecord::Base noteable_type == "Commit" end + def for_commit_diff_line? + for_commit? && for_diff_line? + end + + def for_merge_request? + noteable_type == "MergeRequest" + end + + def for_merge_request_diff_line? + for_merge_request? && for_diff_line? + end + def for_diff_line? line_code.present? end @@ -120,7 +132,7 @@ class Note < ActiveRecord::Base end def diff - target.diffs[diff_index] + noteable.diffs[diff_index] end end diff --git a/app/views/notes/_discussion.html.haml b/app/views/notes/_discussion.html.haml index 991374381ac..28db89004e6 100644 --- a/app/views/notes/_discussion.html.haml +++ b/app/views/notes/_discussion.html.haml @@ -3,17 +3,16 @@ = image_tag gravatar_icon(note.author.email), class: "avatar s32" .text %strong= note.author_name - - case note.noteable_type - - when 'MergeRequest' + - if note.for_merge_request? %cite.cgray started a discussion on this merge request diff = link_to_merge_request_diff_line_note(note) - - when 'Commit' + - elsif note.for_commit? %cite.cgray started a discussion on commit - #{link_to note.target.short_id, project_commit_path(@project, note.target)} - = link_to_commit_diff_line_note(note) if note.line_note? + #{link_to note.noteable.short_id, project_commit_path(@project, note.noteable)} + = link_to_commit_diff_line_note(note) if note.for_diff_line? - else %cite.cgray started a discussion - - if note.line_code? + - if note.for_diff_line? .diff_file .diff_file_header - if diff.deleted_file @@ -24,7 +23,7 @@ %span{id: "#{diff.b_path}"}= diff.b_path %br/ .diff_file_content - = render "commits/text_file", diff: note.diff, index: note.diff_index + = render "commits/text_file", diff: diff, index: note.diff_index - else .body - discussion_notes(note).each do |a_note| diff --git a/app/views/notes/_notes.html.haml b/app/views/notes/_notes.html.haml index b6b1c167d20..7c28983f38e 100644 --- a/app/views/notes/_notes.html.haml +++ b/app/views/notes/_notes.html.haml @@ -1,6 +1,6 @@ - @notes.each do |note| - next unless note.author - - if part_of_discussion?(note) + - if @show_discussions && part_of_discussion?(note) = render_discussion(note) - else = render 'note', note: note -- GitLab From bea8f31f9e4d54c5b173998a48ac55bcf2be8c1d Mon Sep 17 00:00:00 2001 From: Vincent Bonmalais Date: Sun, 21 Oct 2012 13:50:04 +1100 Subject: [PATCH 7/7] Add specs for the discussion feature. Some refactoring has also been done in the helper to limit his role. --- app/helpers/discussion_helper.rb | 32 +++----- app/views/notes/_discussion.html.haml | 2 + app/views/notes/_notes.html.haml | 3 +- spec/factories.rb | 27 ++++++- spec/helpers/discussion_helper_spec.rb | 101 +++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 24 deletions(-) create mode 100644 spec/helpers/discussion_helper_spec.rb diff --git a/app/helpers/discussion_helper.rb b/app/helpers/discussion_helper.rb index 2b72373a905..56daf63c752 100644 --- a/app/helpers/discussion_helper.rb +++ b/app/helpers/discussion_helper.rb @@ -3,29 +3,21 @@ module DiscussionHelper note.for_commit? || note.for_merge_request_diff_line? end - def discussion_title(note) - if note.for_merge_request? - "#{note.author.name} started a discussion on this merge request (on line #{note.diff_new_line}):" - elsif note.for_commit_diff_line? - "#{note.author.name} started a discussion on commit #{link_to_commit_diff_line_note(note)}:" - elsif note.for_commit? - "#{note.author.name} started a discussion on commit #{note.noteable_id}:" - else - "#{note.author.name} started a discussion:" - end + def has_rendered?(note) + discussions_for(note).include?(discussion_id_for(note)) end - def render_discussion(note) - unless discussions_for(note).include?(discussion_id_for(note)) - discussions_for(note).push(discussion_id_for(note)) + def discussion_rendered!(note) + discussions_for(note).push(discussion_id_for(note)) + end - if note.for_diff_line? - @reply_allowed = true - @line_notes = discussion_notes(note) - render 'discussion', note: note, diff: note.diff - else - render 'discussion', note: note - end + def discussion_params(note) + if note.for_diff_line? + @reply_allowed = true + @line_notes = discussion_notes(note) + { note: note, diff: note.diff } + else + { note: note } end end diff --git a/app/views/notes/_discussion.html.haml b/app/views/notes/_discussion.html.haml index 28db89004e6..691566303ad 100644 --- a/app/views/notes/_discussion.html.haml +++ b/app/views/notes/_discussion.html.haml @@ -28,3 +28,5 @@ .body - discussion_notes(note).each do |a_note| = render 'note', note: a_note + +- discussion_rendered!(note) diff --git a/app/views/notes/_notes.html.haml b/app/views/notes/_notes.html.haml index 7c28983f38e..b1c1dfaeef9 100644 --- a/app/views/notes/_notes.html.haml +++ b/app/views/notes/_notes.html.haml @@ -1,6 +1,7 @@ - @notes.each do |note| - next unless note.author - if @show_discussions && part_of_discussion?(note) - = render_discussion(note) + - unless has_rendered?(note) + = render 'discussion', discussion_params(note) - else = render 'note', note: note diff --git a/spec/factories.rb b/spec/factories.rb index 72745c665c4..c6f704bdafc 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -81,9 +81,7 @@ FactoryGirl.define do closed true end - factory :closed_merge_request, traits: [:closed] - - factory :merge_request_with_diffs do + trait :with_diffs do st_commits do repo = Grit::Repo.new(Rails.root.join('tmp', 'repositories', 'gitlabhq')) [Commit.new(repo.commit('bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a'))] @@ -96,11 +94,34 @@ FactoryGirl.define do diffs end end + + factory :closed_merge_request, traits: [:closed] + factory :merge_request_with_diffs, traits: [:with_diffs] end factory :note do project note "Note" + author + + trait :on_line do + line_code "0_184_184" + end + + trait :on_merge_request do + noteable_id 1 + noteable_type "MergeRequest" + end + + trait :on_commit do + noteable_id "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a" + noteable_type "Commit" + end + + factory :note_on_merge_request_line, traits: [:on_merge_request, :on_line] + factory :note_on_merge_request, traits: [:on_merge_request] + factory :note_on_commit_line, traits: [:on_commit, :on_line] + factory :note_on_commit, traits: [:on_commit] end factory :event do diff --git a/spec/helpers/discussion_helper_spec.rb b/spec/helpers/discussion_helper_spec.rb new file mode 100644 index 00000000000..876034252f0 --- /dev/null +++ b/spec/helpers/discussion_helper_spec.rb @@ -0,0 +1,101 @@ +require 'spec_helper' + +describe DiscussionHelper do + let(:project) { FactoryGirl.build(:project) } + + describe "#part_of_discussion" do + let(:note_on_merge_request_line) { FactoryGirl.build(:note_on_merge_request_line, project: project) } + let(:note_on_merge_request) { FactoryGirl.build(:note_on_merge_request, project: project) } + let(:note_on_commit_line) { FactoryGirl.build(:note_on_commit_line, project: project) } + let(:note_on_commit) { FactoryGirl.build(:note_on_commit, project: project) } + + it "should consider a note on a line in a merge request to be a discussion" do + part_of_discussion?(note_on_merge_request_line).should be_true + end + + it "should consider a note on a line in a commit to be a discussion" do + part_of_discussion?(note_on_commit_line).should be_true + end + + it "should consider a note in a commit to be a discussion" do + part_of_discussion?(note_on_commit).should be_true + end + + it "should not consider a note on a merge request to be a discussion" do + part_of_discussion?(note_on_merge_request).should be_false + end + end + + describe "#discussion_notes" do + shared_examples "compare attributes" do + before :each do + @notes = [note_a] + end + + let(:note_a) { FactoryGirl.build(:note_on_merge_request_line, { project: project }.merge(note_spec_a)) } + let(:note_b) { FactoryGirl.build(:note_on_merge_request_line, { project: project }.merge(note_spec_b)) } + + it "should find notes with the same attributes" do + discussion_notes(note_a).size.should == 1 + end + + it "should not find notes with different attributes" do + discussion_notes(note_b).size.should == 0 + end + end + + context "using different line_code attribute" do + let(:note_spec_a) { { line_code: "0_1_1" } } + let(:note_spec_b) { { line_code: "0_2_2" } } + + include_examples "compare attributes" + end + + context "notes with different noteable_id" do + let(:note_spec_a) { { noteable_id: 1 } } + let(:note_spec_b) { { noteable_id: 2 } } + + include_examples "compare attributes" + end + + context "notes with different noteable_type" do + let(:note_spec_a) { { noteable_type: "MergeRequest" } } + let(:note_spec_b) { { noteable_type: "Commit" } } + + include_examples "compare attributes" + end + end + + describe "rendering" do + before :each do + @notes = [] + 3.times do + @notes << FactoryGirl.build(:note_on_merge_request_line, project: project, line_code: "0_1_1") + end + end + + let(:note) { FactoryGirl.build(:note_on_merge_request_line, project: project, line_code: "0_1_1") } + + context "when a note is not rendered yet" do + it "should allow rendering of the discussion" do + has_rendered?(note).should be_false + end + end + + context "once a note has been rendered" do + before :each do + discussion_rendered!(note) + end + + it "should not be rendered anymore" do + has_rendered?(note).should be_true + end + + it "should not render any note in the discussion too" do + @notes.each do |other_note| + has_rendered?(other_note).should be_true + end + end + end + end +end -- GitLab