diff --git a/app/assets/javascripts/merge_requests.js b/app/assets/javascripts/merge_requests.js index 0ab6f6e22a13cf4a7b9742fe43e6ed0d3c456614..561cff663193ea2ea8e49aa19ebf321b7d5f2bd3 100644 --- a/app/assets/javascripts/merge_requests.js +++ b/app/assets/javascripts/merge_requests.js @@ -19,6 +19,8 @@ var MergeRequest = { 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; }); diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 558643d504db3710cd5a13706bfcfe2ba5101120..b561d8fdec3519735dd99bbf96c0c07daef15941 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -263,6 +263,8 @@ var PerLineNotes = { 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; }); diff --git a/app/assets/stylesheets/sections/notes.scss b/app/assets/stylesheets/sections/notes.scss index d24d070df1ea8aab9a6be668fdf190c9f24b4c03..1e611441f5370713bd70cc4bbf4490275540bda1 100644 --- a/app/assets/stylesheets/sections/notes.scss +++ b/app/assets/stylesheets/sections/notes.scss @@ -8,6 +8,41 @@ list-style:none; margin:0px; padding:0px; + + .note_discussion { + .header { + 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; + margin-left: 15px; + + .note { + border-bottom: 0px; + padding: 0px; + } + } + + .body { + .note { + border-bottom: 1px solid #ddd; + padding: 15px; + } + .note:nth-child(odd) { + background-color: #fafafa; + } + } + } } .issue_notes, @@ -73,6 +108,7 @@ #notes-list.reversed .note, #new-notes-list.reversed .note { border-top: 1px solid #eee; + } /* mark vote notes */ diff --git a/app/controllers/commit_controller.rb b/app/controllers/commit_controller.rb index 97998352255ff6e1aa47b64cd2e1031bcf9753e1..89efaf49d81e4e7173014b7f2345f5dcffcafcb9 100644 --- a/app/controllers/commit_controller.rb +++ b/app/controllers/commit_controller.rb @@ -17,7 +17,9 @@ class CommitController < ProjectResourceController @note = result[:note] @line_notes = result[:line_notes] @notes_count = result[:notes_count] - @comments_allowed = true + @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 8e180c9baae7cd78129755f84ceced37d4bc54cc..efa04e67a79f7f8b0021dd9f8a479c135400231c 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -36,7 +36,9 @@ class MergeRequestsController < ProjectResourceController @diffs = @merge_request.diffs @commit = @merge_request.last_commit - @comments_allowed = true + @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 d794f368f577b29f7ecc8863e9ab02a5258c8876..2dc0abf546e64623485381ba3fd7ab34c7a35b3d 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -7,9 +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) diff --git a/app/helpers/discussion_helper.rb b/app/helpers/discussion_helper.rb new file mode 100644 index 0000000000000000000000000000000000000000..56daf63c7529595ec40f9a8f13a98e29c4b0597f --- /dev/null +++ b/app/helpers/discussion_helper.rb @@ -0,0 +1,45 @@ +module DiscussionHelper + def part_of_discussion?(note) + note.for_commit? || note.for_merge_request_diff_line? + end + + def has_rendered?(note) + discussions_for(note).include?(discussion_id_for(note)) + end + + def discussion_rendered!(note) + discussions_for(note).push(discussion_id_for(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 + + 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(note) + @discussions ||= { merge_requests: [], commits: [] } + + if note.for_merge_request? + @discussions[:merge_requests] + elsif note.for_commit? + @discussions[:commits] + else + [] + end + end +end diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index ffcc7acc8da71cd2985c5cd635da281a36ed43a9..dbcd2a918edb48b812508eca874d4f58bd522114 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 @@ -13,12 +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 + 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 - link_to "#{link_file}:L#{link_line}", project_commit_path(@project, commit, anchor: note.line_code) + def link_to_merge_request_diff_line_note(note) + 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 e2f4a89d7d6187210765b36e2c6b6aad355fef04..8b7d6b443d49fa006b2203ed8769f24d2c5f368a 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 @@ -106,6 +118,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 + noteable.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 02117386d71c93a024b3c438771e0144dcfe5236..5e7886922b4720bed3d47616155ee29ada70d4d8 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/merge_requests/_show.html.haml b/app/views/merge_requests/_show.html.haml index f1d0c8aaafbbbcaa1551452c5bfdcfd837d7f721..d12431da70b0e434d76855516a4aa7cfe887e38c 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/diffs.html.haml b/app/views/merge_requests/diffs.html.haml index a755491c42ef91d96da908424c071cc1205c0d45..2a5b8b1441ef89345d4d5357b4f5b557d80fe7e7 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/merge_requests/show.js.haml b/app/views/merge_requests/show.js.haml index f44ccbb5127a52c7ea8031de48b588b19dc66c07..e624ce51d78b883cb836338bab83a7b4a612d080 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/_create_per_line_note.js.haml b/app/views/notes/_create_per_line_note.js.haml index 180960e38e4e357afa0429b92f6c677309ae3aac..eea9eb38be4cffe0a606278caaaa07084b092ab2 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/_discussion.html.haml b/app/views/notes/_discussion.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..691566303ad9e4bf07f4b74c630d3d8bf34551ea --- /dev/null +++ b/app/views/notes/_discussion.html.haml @@ -0,0 +1,32 @@ +.note_discussion + .header + = image_tag gravatar_icon(note.author.email), class: "avatar s32" + .text + %strong= note.author_name + - if note.for_merge_request? + %cite.cgray started a discussion on this merge request diff + = link_to_merge_request_diff_line_note(note) + - elsif note.for_commit? + %cite.cgray started a discussion on commit + #{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.for_diff_line? + .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: diff, index: note.diff_index + - else + .body + - discussion_notes(note).each do |a_note| + = render 'note', note: a_note + +- discussion_rendered!(note) diff --git a/app/views/notes/_note.html.haml b/app/views/notes/_note.html.haml index 70baa212d1002d787ab6f6e28346f86d77f3039f..57946163b567f26374ba430bdfdabf5b01b572e8 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 adb5dfcbf18741de0e635f3958670f0d65a88be2..b1c1dfaeef90ecb93f9eb68cd9adfc2637911b8a 100644 --- a/app/views/notes/_notes.html.haml +++ b/app/views/notes/_notes.html.haml @@ -1,4 +1,7 @@ - @notes.each do |note| - next unless note.author - = render "note", note: note - + - if @show_discussions && part_of_discussion?(note) + - unless has_rendered?(note) + = render 'discussion', discussion_params(note) + - else + = render 'note', 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 72b59a596a352d02897948f85f4ddd3fd742991f..0a4d2e76a088a49f74d7d781fb7f7327788f6f62 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: @comments_target.merge({ line_code: line_code }), title: "Add note on line #{line_code[/[0-9]+\z/]}" 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 1bcfc41fe203b81162ef6447ee8c8bca8ab7f37e..ebe0c5c6c4709a1c85f69cafc94320ee90906d61 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 42c737c75ae130e31ab518020ae427a4a0f7be79..ac3884a328cd410d9a143009cf676b0ef819f31b 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 3814dbd46a2aa05a5eef17510e18d1ece396700a..99da619c649c88585fa1c08ebda9750ef64f5946 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(); diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 80f009864662be4375ba8cf24577d8ec43bde52a..5b8becbb6c9abdb47e9689b3869b44dff6f5f542 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -35,8 +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 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 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 80e83906c723e93ff069857f7b29fd3e4982e500..3b9464c77d654bd120a0986db35c6596f907e097 100644 --- a/features/steps/project/project_merge_requests.rb +++ b/features/steps/project/project_merge_requests.rb @@ -4,77 +4,148 @@ 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 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 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 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 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}: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 diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index eda5ab9411636922b955c14f4930984c51cb2f26..d401e916175e26ae5e5eb9faffb6f9d24ce7af65 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 0258f8920e9ad56b98387fa8274cd9d251d79019..c6f704bdafc932ba3f06a5f21131295381198e99 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -81,12 +81,47 @@ FactoryGirl.define do closed true end + trait :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 + 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 0000000000000000000000000000000000000000..876034252f0d5eedd1347303f5507659f41a73e5 --- /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