From 10b8c62b8617e4e2a648f07505701caac3abca64 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Thu, 21 Jul 2016 12:45:49 +0100 Subject: Added new spec descriptions and scenarios --- spec/features/merge_requests/diffs_spec.rb | 169 +++++++++++++++++++++++++++++ 1 file changed, 169 insertions(+) diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb index c9a0059645d..9ac08b06da8 100644 --- a/spec/features/merge_requests/diffs_spec.rb +++ b/spec/features/merge_requests/diffs_spec.rb @@ -22,4 +22,173 @@ feature 'Diffs URL', js: true, feature: true do expect(page).to have_css('.diffs.tab-pane.active') end end + + context 'when hovering over the parallel view diff file' do + let(:comment_button_class) { '.add-diff-note' } + + before(:each) do + visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + click_link 'Side-by-side' + # @old_line_number = first '.diff-line-num.old_line:not(.empty-cell)' + # @new_line_number = first '.diff-line-num.new_line:not(.empty-cell)' + # @old_line = first '.line_content[data-line-type="old"]' + # @new_line = first '.line_content[data-line-type="new"]' + end + + context 'with an old line on the left and no line on the right' do + it 'should allow commenting on the left side' do + puts first('//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")] and child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," new ")]]') + expect(page).to have_content 'NOPE' + end + + it 'should not allow commenting on the right side' do + + end + end + + context 'with no line on the left and a new line on the right' do + it 'should allow commenting on the right side' do + + end + + it 'should not allow commenting on the left side' do + + end + end + + context 'with an old line on the left and a new line on the right' do + it 'should allow commenting on the left side' do + + end + + it 'should allow commenting on the right side' do + + end + end + + context 'with an unchanged line on the left and an unchanged line on the right' do + it 'should allow commenting on the left side' do + + end + + it 'should allow commenting on the right side' do + + end + end + + context 'with a match line' do + it 'should not allow commenting on the left side' do + + end + + it 'should not allow commenting on the right side' do + + end + end + + # it 'shows a comment button on the old side when hovering over an old line number' do + # @old_line_number.hover + # expect(@old_line_number).to have_css comment_button_class + # expect(@new_line_number).not_to have_css comment_button_class + # end + # + # it 'shows a comment button on the old side when hovering over an old line' do + # @old_line.hover + # expect(@old_line_number).to have_css comment_button_class + # expect(@new_line_number).not_to have_css comment_button_class + # end + # + # it 'shows a comment button on the new side when hovering over a new line number' do + # @new_line_number.hover + # expect(@new_line_number).to have_css comment_button_class + # expect(@old_line_number).not_to have_css comment_button_class + # end + # + # it 'shows a comment button on the new side when hovering over a new line' do + # @new_line.hover + # expect(@new_line_number).to have_css comment_button_class + # expect(@old_line_number).not_to have_css comment_button_class + # end + end + + context 'when hovering over the inline view diff file' do + let(:comment_button_class) { '.add-diff-note' } + + before(:each) do + visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + click_link 'Inline' + # @old_line_number = first '.diff-line-num.old_line:not(.unfold)' + # @new_line_number = first '.diff-line-num.new_line:not(.unfold)' + # @new_line = first '.line_content:not(.match)' + end + + context 'with a new line' do + it 'should allow commenting' do + + end + end + + context 'with an old line' do + it 'should allow commenting' do + + end + end + + context 'with an unchanged line' do + it 'should allow commenting' do + + end + end + + context 'with a match line' do + it 'should not allow commenting' do + + end + end + + # it 'shows a comment button on the old side when hovering over an old line number' do + # @old_line_number.hover + # expect(@old_line_number).to have_css comment_button_class + # expect(@new_line_number).not_to have_css comment_button_class + # end + # + # it 'shows a comment button on the new side when hovering over a new line number' do + # @new_line_number.hover + # expect(@old_line_number).to have_css comment_button_class + # expect(@new_line_number).not_to have_css comment_button_class + # end + # + # it 'shows a comment button on the new side when hovering over a new line' do + # @new_line.hover + # expect(@old_line_number).to have_css comment_button_class + # expect(@new_line_number).not_to have_css comment_button_class + # end + end + + # context 'when clicking a comment button' do + # let(:test_note_comment) { 'this is a test note!' } + # let(:note_class) { '.new-note' } + # + # before(:each) do + # visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + # click_link 'Inline' + # first('.diff-line-num.old_line:not(.unfold)').hover + # find('.add-diff-note').click + # end + # + # it 'shows a note form' do + # expect(page).to have_css note_class + # end + # + # it 'can be submitted and viewed' do + # fill_in 'note[note]', with: test_note_comment + # click_button 'Comment' + # expect(page).to have_content test_note_comment + # end + # + # it 'can be closed' do + # find('.note-form-actions .btn-cancel').click + # expect(page).not_to have_css note_class + # end + # end end -- cgit v1.2.1 From dd79472bacd73fc3ece7129539e20d8bc78e22f1 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Fri, 22 Jul 2016 06:51:58 +0100 Subject: Finished up intial version that uses XPath extensively --- spec/features/merge_requests/diffs_spec.rb | 235 +++++++++++++---------------- 1 file changed, 109 insertions(+), 126 deletions(-) diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb index 9ac08b06da8..ae237ad5e3f 100644 --- a/spec/features/merge_requests/diffs_spec.rb +++ b/spec/features/merge_requests/diffs_spec.rb @@ -23,172 +23,155 @@ feature 'Diffs URL', js: true, feature: true do end end - context 'when hovering over the parallel view diff file' do + context 'diff notes' do let(:comment_button_class) { '.add-diff-note' } - - before(:each) do - visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) - click_link 'Side-by-side' - # @old_line_number = first '.diff-line-num.old_line:not(.empty-cell)' - # @new_line_number = first '.diff-line-num.new_line:not(.empty-cell)' - # @old_line = first '.line_content[data-line-type="old"]' - # @new_line = first '.line_content[data-line-type="new"]' - end - - context 'with an old line on the left and no line on the right' do - it 'should allow commenting on the left side' do - puts first('//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")] and child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," new ")]]') - expect(page).to have_content 'NOPE' + let(:notes_holder_input_class) { 'js-temp-notes-holder' } + let(:notes_holder_input_xpath) { './following-sibling::*[contains(concat(" ", @class, " "), " notes_holder ")]' } + let(:test_note_comment) { 'this is a test note!' } + # line_holder = //*[contains(concat(" ", @class, " "), " line_holder "] + # old_line = child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")] + # new_line = child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," new ")] + # match_line = child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," match ")] + # unchanged_line = child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])] + # no_line = child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and not(boolean(node()[1]))] + + context 'when hovering over the parallel view diff file' do + before(:each) do + visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + click_link 'Side-by-side' end - it 'should not allow commenting on the right side' do - - end - end + context 'with an old line on the left and no line on the right' do + let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")] and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and not(boolean(node()[1]))]]' } - context 'with no line on the left and a new line on the right' do - it 'should allow commenting on the right side' do + it 'should allow commenting on the left side' do + should_allow_commenting line_holder, 'left' + end + it 'should not allow commenting on the right side' do + should_not_allow_commenting line_holder, 'right' + end end - it 'should not allow commenting on the left side' do + context 'with no line on the left and a new line on the right' do + let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and not(boolean(node()[1]))] and child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," new ")]]' } - end - end - - context 'with an old line on the left and a new line on the right' do - it 'should allow commenting on the left side' do + it 'should not allow commenting on the left side' do + should_not_allow_commenting line_holder, 'left' + end + it 'should allow commenting on the right side' do + should_allow_commenting line_holder, 'right' + end end - it 'should allow commenting on the right side' do + context 'with an old line on the left and a new line on the right' do + let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")] and child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," new ")]]' } + + it 'should allow commenting on the left side' do + should_allow_commenting line_holder, 'left' + end + it 'should allow commenting on the right side' do + should_allow_commenting line_holder, 'right' + end end - end - context 'with an unchanged line on the left and an unchanged line on the right' do - it 'should allow commenting on the left side' do + context 'with an unchanged line on the left and an unchanged line on the right' do + let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])] and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])]]' } + + it 'should allow commenting on the left side' do + should_allow_commenting line_holder, 'left' + end + it 'should allow commenting on the right side' do + should_allow_commenting line_holder, 'right' + end end - it 'should allow commenting on the right side' do + context 'with a match line' do + let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " match ")] and child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," match ")]]' } + + it 'should not allow commenting on the left side' do + should_not_allow_commenting line_holder, 'left' + end + it 'should not allow commenting on the right side' do + should_not_allow_commenting line_holder, 'right' + end end end - context 'with a match line' do - it 'should not allow commenting on the left side' do + context 'when hovering over the inline view diff file' do + let(:comment_button_class) { '.add-diff-note' } + before(:each) do + visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + click_link 'Inline' end - it 'should not allow commenting on the right side' do + context 'with a new line' do + let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " new ")]]' } + it 'should allow commenting' do + should_allow_commenting line_holder + end end - end - - # it 'shows a comment button on the old side when hovering over an old line number' do - # @old_line_number.hover - # expect(@old_line_number).to have_css comment_button_class - # expect(@new_line_number).not_to have_css comment_button_class - # end - # - # it 'shows a comment button on the old side when hovering over an old line' do - # @old_line.hover - # expect(@old_line_number).to have_css comment_button_class - # expect(@new_line_number).not_to have_css comment_button_class - # end - # - # it 'shows a comment button on the new side when hovering over a new line number' do - # @new_line_number.hover - # expect(@new_line_number).to have_css comment_button_class - # expect(@old_line_number).not_to have_css comment_button_class - # end - # - # it 'shows a comment button on the new side when hovering over a new line' do - # @new_line.hover - # expect(@new_line_number).to have_css comment_button_class - # expect(@old_line_number).not_to have_css comment_button_class - # end - end - context 'when hovering over the inline view diff file' do - let(:comment_button_class) { '.add-diff-note' } + context 'with an old line' do + let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")]]' } - before(:each) do - visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) - click_link 'Inline' - # @old_line_number = first '.diff-line-num.old_line:not(.unfold)' - # @new_line_number = first '.diff-line-num.new_line:not(.unfold)' - # @new_line = first '.line_content:not(.match)' - end + it 'should allow commenting' do + should_allow_commenting line_holder + end + end - context 'with a new line' do - it 'should allow commenting' do + context 'with an unchanged line' do + let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])] and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])]]' } + it 'should allow commenting' do + should_allow_commenting line_holder + end end - end - context 'with an old line' do - it 'should allow commenting' do + context 'with a match line' do + let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " match ")]]' } + it 'should not allow commenting' do + should_not_allow_commenting line_holder + end end end - context 'with an unchanged line' do - it 'should allow commenting' do - - end + def should_allow_commenting(line_holder, diff_side = nil) + line = get_line diff_side + line[:content].hover + expect(line[:num]).to have_css comment_button_class + line[:num].find(comment_button_class).trigger 'click' + expect(line_holder).to have_xpath notes_holder_input_xpath + notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath) + expect(notes_holder_input[:class].include? notes_holder_input_class).to be true + notes_holder_input.fill_in 'note[note]', with: test_note_comment + click_button 'Comment' + expect(line_holder).to have_xpath notes_holder_input_xpath + notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) + expect(notes_holder_saved[:class].include? notes_holder_input_class).to be false + expect(notes_holder_saved).to have_content test_note_comment end - context 'with a match line' do - it 'should not allow commenting' do + def should_not_allow_commenting(line_holder, diff_side = nil) + line = get_line diff_side + line[:content].hover + expect(line[:num]).not_to have_css comment_button_class + end + def get_line(diff_side = nil) + if diff_side.nil? + { content: line_holder.first('.line_content'), num: line_holder.first('.diff-line-num') } + else + side_index = diff_side == 'left' ? 0 : 1 + { content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] } end end - - # it 'shows a comment button on the old side when hovering over an old line number' do - # @old_line_number.hover - # expect(@old_line_number).to have_css comment_button_class - # expect(@new_line_number).not_to have_css comment_button_class - # end - # - # it 'shows a comment button on the new side when hovering over a new line number' do - # @new_line_number.hover - # expect(@old_line_number).to have_css comment_button_class - # expect(@new_line_number).not_to have_css comment_button_class - # end - # - # it 'shows a comment button on the new side when hovering over a new line' do - # @new_line.hover - # expect(@old_line_number).to have_css comment_button_class - # expect(@new_line_number).not_to have_css comment_button_class - # end end - - # context 'when clicking a comment button' do - # let(:test_note_comment) { 'this is a test note!' } - # let(:note_class) { '.new-note' } - # - # before(:each) do - # visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) - # click_link 'Inline' - # first('.diff-line-num.old_line:not(.unfold)').hover - # find('.add-diff-note').click - # end - # - # it 'shows a note form' do - # expect(page).to have_css note_class - # end - # - # it 'can be submitted and viewed' do - # fill_in 'note[note]', with: test_note_comment - # click_button 'Comment' - # expect(page).to have_content test_note_comment - # end - # - # it 'can be closed' do - # find('.note-form-actions .btn-cancel').click - # expect(page).not_to have_css note_class - # end - # end end -- cgit v1.2.1 From 59ddf726a6866483c4ec9beaf27423206e3ed3e9 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Fri, 22 Jul 2016 16:48:50 +0100 Subject: Fixed failing tests with WaitForAjax --- spec/features/merge_requests/diffs_spec.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb index ae237ad5e3f..dbc50ea5265 100644 --- a/spec/features/merge_requests/diffs_spec.rb +++ b/spec/features/merge_requests/diffs_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' feature 'Diffs URL', js: true, feature: true do + include WaitForAjax + before do login_as :admin @merge_request = create(:merge_request) @@ -153,6 +155,7 @@ feature 'Diffs URL', js: true, feature: true do expect(notes_holder_input[:class].include? notes_holder_input_class).to be true notes_holder_input.fill_in 'note[note]', with: test_note_comment click_button 'Comment' + wait_for_ajax expect(line_holder).to have_xpath notes_holder_input_xpath notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) expect(notes_holder_saved[:class].include? notes_holder_input_class).to be false -- cgit v1.2.1 From ef5a5d0ba30709f4e146ee31441d092759c32911 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Mon, 25 Jul 2016 04:34:49 +0100 Subject: Tidying up spec for new implementation of css ID selectors --- spec/features/merge_requests/diffs_spec.rb | 60 +++++++++++++++++++----------- 1 file changed, 38 insertions(+), 22 deletions(-) diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb index dbc50ea5265..f5e8677561f 100644 --- a/spec/features/merge_requests/diffs_spec.rb +++ b/spec/features/merge_requests/diffs_spec.rb @@ -30,12 +30,12 @@ feature 'Diffs URL', js: true, feature: true do let(:notes_holder_input_class) { 'js-temp-notes-holder' } let(:notes_holder_input_xpath) { './following-sibling::*[contains(concat(" ", @class, " "), " notes_holder ")]' } let(:test_note_comment) { 'this is a test note!' } - # line_holder = //*[contains(concat(" ", @class, " "), " line_holder "] - # old_line = child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")] - # new_line = child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," new ")] - # match_line = child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," match ")] - # unchanged_line = child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])] - # no_line = child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and not(boolean(node()[1]))] + # line_holder = + # old_line = + # new_line = + # match_line = + # unchanged_line = + # no_line = context 'when hovering over the parallel view diff file' do before(:each) do @@ -146,35 +146,51 @@ feature 'Diffs URL', js: true, feature: true do end def should_allow_commenting(line_holder, diff_side = nil) - line = get_line diff_side + line = get_line_components line_holder, diff_side line[:content].hover expect(line[:num]).to have_css comment_button_class - line[:num].find(comment_button_class).trigger 'click' - expect(line_holder).to have_xpath notes_holder_input_xpath - notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_input[:class].include? notes_holder_input_class).to be true - notes_holder_input.fill_in 'note[note]', with: test_note_comment - click_button 'Comment' + comment_on_line line_holder, line wait_for_ajax - expect(line_holder).to have_xpath notes_holder_input_xpath - notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_saved[:class].include? notes_holder_input_class).to be false - expect(notes_holder_saved).to have_content test_note_comment + assert_comment_persistence end def should_not_allow_commenting(line_holder, diff_side = nil) - line = get_line diff_side + line = get_line_components line_holder, diff_side line[:content].hover expect(line[:num]).not_to have_css comment_button_class end - def get_line(diff_side = nil) + def get_line_components(line_holder, diff_side = nil) if diff_side.nil? - { content: line_holder.first('.line_content'), num: line_holder.first('.diff-line-num') } + get_inline_line_components line_holder else - side_index = diff_side == 'left' ? 0 : 1 - { content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] } + get_parallel_line_components line_holder, diff_side end end + + def get_inline_line_components(line_holder) + { content: line_holder.first('.line_content'), num: line_holder.first('.diff-line-num') } + end + + def get_parallel_line_components(line_holder, diff_side = nil) + side_index = diff_side == 'left' ? 0 : 1 + { content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] } + end + + def comment_on_line(line_holder, line) + line[:num].find(comment_button_class).trigger 'click' + expect(line_holder).to have_xpath notes_holder_input_xpath + notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath) + expect(notes_holder_input[:class].include? notes_holder_input_class).to be true + notes_holder_input.fill_in 'note[note]', with: test_note_comment + click_button 'Comment' + end + + def assert_comment_persistence(line_holder) + expect(line_holder).to have_xpath notes_holder_input_xpath + notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) + expect(notes_holder_saved[:class].include? notes_holder_input_class).to be false + expect(notes_holder_saved).to have_content test_note_comment + end end end -- cgit v1.2.1 From f9806bdef291113d6112769093020af3dcb1000c Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Mon, 25 Jul 2016 05:52:26 +0100 Subject: Finished css replacement of xpath selectors and tidying up spec --- spec/features/merge_requests/diffs_spec.rb | 31 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb index f5e8677561f..d93fc5e84ee 100644 --- a/spec/features/merge_requests/diffs_spec.rb +++ b/spec/features/merge_requests/diffs_spec.rb @@ -30,12 +30,6 @@ feature 'Diffs URL', js: true, feature: true do let(:notes_holder_input_class) { 'js-temp-notes-holder' } let(:notes_holder_input_xpath) { './following-sibling::*[contains(concat(" ", @class, " "), " notes_holder ")]' } let(:test_note_comment) { 'this is a test note!' } - # line_holder = - # old_line = - # new_line = - # match_line = - # unchanged_line = - # no_line = context 'when hovering over the parallel view diff file' do before(:each) do @@ -44,7 +38,7 @@ feature 'Diffs URL', js: true, feature: true do end context 'with an old line on the left and no line on the right' do - let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")] and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and not(boolean(node()[1]))]]' } + let(:line_holder) { find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..') } it 'should allow commenting on the left side' do should_allow_commenting line_holder, 'left' @@ -56,7 +50,7 @@ feature 'Diffs URL', js: true, feature: true do end context 'with no line on the left and a new line on the right' do - let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and not(boolean(node()[1]))] and child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," new ")]]' } + let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..') } it 'should not allow commenting on the left side' do should_not_allow_commenting line_holder, 'left' @@ -68,7 +62,7 @@ feature 'Diffs URL', js: true, feature: true do end context 'with an old line on the left and a new line on the right' do - let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")] and child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," new ")]]' } + let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..') } it 'should allow commenting on the left side' do should_allow_commenting line_holder, 'left' @@ -80,7 +74,7 @@ feature 'Diffs URL', js: true, feature: true do end context 'with an unchanged line on the left and an unchanged line on the right' do - let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])] and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])]]' } + let(:line_holder) { first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..') } it 'should allow commenting on the left side' do should_allow_commenting line_holder, 'left' @@ -92,7 +86,7 @@ feature 'Diffs URL', js: true, feature: true do end context 'with a match line' do - let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " match ")] and child::*[contains(concat(" ", @class, " ")," line_content ") and contains(concat(" ", @class, " ")," match ")]]' } + let(:line_holder) { first('.match').find(:xpath, '..') } it 'should not allow commenting on the left side' do should_not_allow_commenting line_holder, 'left' @@ -113,7 +107,7 @@ feature 'Diffs URL', js: true, feature: true do end context 'with a new line' do - let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " new ")]]' } + let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]') } it 'should allow commenting' do should_allow_commenting line_holder @@ -121,7 +115,7 @@ feature 'Diffs URL', js: true, feature: true do end context 'with an old line' do - let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " old ")]]' } + let(:line_holder) { find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]') } it 'should allow commenting' do should_allow_commenting line_holder @@ -129,7 +123,7 @@ feature 'Diffs URL', js: true, feature: true do end context 'with an unchanged line' do - let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])] and child::*[contains(concat(" ", @class, " ")," line_content ") and not(contains(concat(" ", @class, " ")," old ")) and not(contains(concat(" ", @class, " ")," new ")) and not(contains(concat(" ", @class, " ")," match ")) and boolean(node()[1])]]' } + let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]') } it 'should allow commenting' do should_allow_commenting line_holder @@ -137,7 +131,7 @@ feature 'Diffs URL', js: true, feature: true do end context 'with a match line' do - let(:line_holder) { first :xpath, '//*[contains(concat(" ", @class, " "), " line_holder ") and child::*[contains(concat(" ", @class, " "), " line_content ") and contains(concat(" ", @class, " "), " match ")]]' } + let(:line_holder) { first('.match') } it 'should not allow commenting' do should_not_allow_commenting line_holder @@ -149,9 +143,11 @@ feature 'Diffs URL', js: true, feature: true do line = get_line_components line_holder, diff_side line[:content].hover expect(line[:num]).to have_css comment_button_class + comment_on_line line_holder, line wait_for_ajax - assert_comment_persistence + + assert_comment_persistence line_holder end def should_not_allow_commenting(line_holder, diff_side = nil) @@ -180,14 +176,17 @@ feature 'Diffs URL', js: true, feature: true do def comment_on_line(line_holder, line) line[:num].find(comment_button_class).trigger 'click' expect(line_holder).to have_xpath notes_holder_input_xpath + notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath) expect(notes_holder_input[:class].include? notes_holder_input_class).to be true + notes_holder_input.fill_in 'note[note]', with: test_note_comment click_button 'Comment' end def assert_comment_persistence(line_holder) expect(line_holder).to have_xpath notes_holder_input_xpath + notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) expect(notes_holder_saved[:class].include? notes_holder_input_class).to be false expect(notes_holder_saved).to have_content test_note_comment -- cgit v1.2.1 From 0013e59fef7fa21e1f24796ad5c97973bf04e0e3 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Fri, 5 Aug 2016 13:56:26 +0100 Subject: Moved notes scenarios to 'diff_notes_spec.rb' --- features/project/merge_requests.feature | 10 -- spec/features/merge_requests/diff_notes_spec.rb | 179 ++++++++++++++++++++++++ spec/features/merge_requests/diffs_spec.rb | 170 ---------------------- 3 files changed, 179 insertions(+), 180 deletions(-) create mode 100644 spec/features/merge_requests/diff_notes_spec.rb diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 21768c15c17..5c1a0099a58 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -106,16 +106,6 @@ Feature: Project Merge Requests And I sort the list by "Least popular" Then The list should be sorted by "Least popular" - @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 click on the Changes tab - And I leave a comment like "Line is wrong" on diff - And I switch to the merge request's comments tab - Then I should see a discussion has started on diff - And I should see a badge of "1" next to the discussion link - @javascript Scenario: I see a new comment on merge request diff from another user in the discussion tab Given project "Shop" have "Bug NS-05" open merge request with diffs inside diff --git a/spec/features/merge_requests/diff_notes_spec.rb b/spec/features/merge_requests/diff_notes_spec.rb new file mode 100644 index 00000000000..8b01601b004 --- /dev/null +++ b/spec/features/merge_requests/diff_notes_spec.rb @@ -0,0 +1,179 @@ +require 'spec_helper' + +feature 'Diff notes', js: true, feature: true do + include WaitForAjax + + before do + login_as :admin + @merge_request = create(:merge_request) + @project = @merge_request.source_project + end + + context 'merge request diffs' do + let(:comment_button_class) { '.add-diff-note' } + let(:notes_holder_input_class) { 'js-temp-notes-holder' } + let(:notes_holder_input_xpath) { './following-sibling::*[contains(concat(" ", @class, " "), " notes_holder ")]' } + let(:test_note_comment) { 'this is a test note!' } + + context 'when hovering over the parallel view diff file' do + before(:each) do + visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + click_link 'Side-by-side' + end + + context 'with an old line on the left and no line on the right' do + let(:line_holder) { find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..') } + + it 'should allow commenting on the left side' do + should_allow_commenting line_holder, 'left' + end + + it 'should not allow commenting on the right side' do + should_not_allow_commenting line_holder, 'right' + end + end + + context 'with no line on the left and a new line on the right' do + let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..') } + + it 'should not allow commenting on the left side' do + should_not_allow_commenting line_holder, 'left' + end + + it 'should allow commenting on the right side' do + should_allow_commenting line_holder, 'right' + end + end + + context 'with an old line on the left and a new line on the right' do + let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..') } + + it 'should allow commenting on the left side' do + should_allow_commenting line_holder, 'left' + end + + it 'should allow commenting on the right side' do + should_allow_commenting line_holder, 'right' + end + end + + context 'with an unchanged line on the left and an unchanged line on the right' do + let(:line_holder) { first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..') } + + it 'should allow commenting on the left side' do + should_allow_commenting line_holder, 'left' + end + + it 'should allow commenting on the right side' do + should_allow_commenting line_holder, 'right' + end + end + + context 'with a match line' do + let(:line_holder) { first('.match').find(:xpath, '..') } + + it 'should not allow commenting on the left side' do + should_not_allow_commenting line_holder, 'left' + end + + it 'should not allow commenting on the right side' do + should_not_allow_commenting line_holder, 'right' + end + end + end + + context 'when hovering over the inline view diff file' do + let(:comment_button_class) { '.add-diff-note' } + + before(:each) do + visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + click_link 'Inline' + end + + context 'with a new line' do + let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]') } + + it 'should allow commenting' do + should_allow_commenting line_holder + end + end + + context 'with an old line' do + let(:line_holder) { find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]') } + + it 'should allow commenting' do + should_allow_commenting line_holder + end + end + + context 'with an unchanged line' do + let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]') } + + it 'should allow commenting' do + should_allow_commenting line_holder + end + end + + context 'with a match line' do + let(:line_holder) { first('.match') } + + it 'should not allow commenting' do + should_not_allow_commenting line_holder + end + end + end + + def should_allow_commenting(line_holder, diff_side = nil) + line = get_line_components line_holder, diff_side + line[:content].hover + expect(line[:num]).to have_css comment_button_class + + comment_on_line line_holder, line + wait_for_ajax + + assert_comment_persistence line_holder + end + + def should_not_allow_commenting(line_holder, diff_side = nil) + line = get_line_components line_holder, diff_side + line[:content].hover + expect(line[:num]).not_to have_css comment_button_class + end + + def get_line_components(line_holder, diff_side = nil) + if diff_side.nil? + get_inline_line_components line_holder + else + get_parallel_line_components line_holder, diff_side + end + end + + def get_inline_line_components(line_holder) + { content: line_holder.first('.line_content'), num: line_holder.first('.diff-line-num') } + end + + def get_parallel_line_components(line_holder, diff_side = nil) + side_index = diff_side == 'left' ? 0 : 1 + { content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] } + end + + def comment_on_line(line_holder, line) + line[:num].find(comment_button_class).trigger 'click' + expect(line_holder).to have_xpath notes_holder_input_xpath + + notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath) + expect(notes_holder_input[:class].include? notes_holder_input_class).to be true + + notes_holder_input.fill_in 'note[note]', with: test_note_comment + click_button 'Comment' + end + + def assert_comment_persistence(line_holder) + expect(line_holder).to have_xpath notes_holder_input_xpath + + notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) + expect(notes_holder_saved[:class].include? notes_holder_input_class).to be false + expect(notes_holder_saved).to have_content test_note_comment + end + end +end diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb index d93fc5e84ee..c9a0059645d 100644 --- a/spec/features/merge_requests/diffs_spec.rb +++ b/spec/features/merge_requests/diffs_spec.rb @@ -1,8 +1,6 @@ require 'spec_helper' feature 'Diffs URL', js: true, feature: true do - include WaitForAjax - before do login_as :admin @merge_request = create(:merge_request) @@ -24,172 +22,4 @@ feature 'Diffs URL', js: true, feature: true do expect(page).to have_css('.diffs.tab-pane.active') end end - - context 'diff notes' do - let(:comment_button_class) { '.add-diff-note' } - let(:notes_holder_input_class) { 'js-temp-notes-holder' } - let(:notes_holder_input_xpath) { './following-sibling::*[contains(concat(" ", @class, " "), " notes_holder ")]' } - let(:test_note_comment) { 'this is a test note!' } - - context 'when hovering over the parallel view diff file' do - before(:each) do - visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) - click_link 'Side-by-side' - end - - context 'with an old line on the left and no line on the right' do - let(:line_holder) { find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..') } - - it 'should allow commenting on the left side' do - should_allow_commenting line_holder, 'left' - end - - it 'should not allow commenting on the right side' do - should_not_allow_commenting line_holder, 'right' - end - end - - context 'with no line on the left and a new line on the right' do - let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..') } - - it 'should not allow commenting on the left side' do - should_not_allow_commenting line_holder, 'left' - end - - it 'should allow commenting on the right side' do - should_allow_commenting line_holder, 'right' - end - end - - context 'with an old line on the left and a new line on the right' do - let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..') } - - it 'should allow commenting on the left side' do - should_allow_commenting line_holder, 'left' - end - - it 'should allow commenting on the right side' do - should_allow_commenting line_holder, 'right' - end - end - - context 'with an unchanged line on the left and an unchanged line on the right' do - let(:line_holder) { first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..') } - - it 'should allow commenting on the left side' do - should_allow_commenting line_holder, 'left' - end - - it 'should allow commenting on the right side' do - should_allow_commenting line_holder, 'right' - end - end - - context 'with a match line' do - let(:line_holder) { first('.match').find(:xpath, '..') } - - it 'should not allow commenting on the left side' do - should_not_allow_commenting line_holder, 'left' - end - - it 'should not allow commenting on the right side' do - should_not_allow_commenting line_holder, 'right' - end - end - end - - context 'when hovering over the inline view diff file' do - let(:comment_button_class) { '.add-diff-note' } - - before(:each) do - visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) - click_link 'Inline' - end - - context 'with a new line' do - let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]') } - - it 'should allow commenting' do - should_allow_commenting line_holder - end - end - - context 'with an old line' do - let(:line_holder) { find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]') } - - it 'should allow commenting' do - should_allow_commenting line_holder - end - end - - context 'with an unchanged line' do - let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]') } - - it 'should allow commenting' do - should_allow_commenting line_holder - end - end - - context 'with a match line' do - let(:line_holder) { first('.match') } - - it 'should not allow commenting' do - should_not_allow_commenting line_holder - end - end - end - - def should_allow_commenting(line_holder, diff_side = nil) - line = get_line_components line_holder, diff_side - line[:content].hover - expect(line[:num]).to have_css comment_button_class - - comment_on_line line_holder, line - wait_for_ajax - - assert_comment_persistence line_holder - end - - def should_not_allow_commenting(line_holder, diff_side = nil) - line = get_line_components line_holder, diff_side - line[:content].hover - expect(line[:num]).not_to have_css comment_button_class - end - - def get_line_components(line_holder, diff_side = nil) - if diff_side.nil? - get_inline_line_components line_holder - else - get_parallel_line_components line_holder, diff_side - end - end - - def get_inline_line_components(line_holder) - { content: line_holder.first('.line_content'), num: line_holder.first('.diff-line-num') } - end - - def get_parallel_line_components(line_holder, diff_side = nil) - side_index = diff_side == 'left' ? 0 : 1 - { content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] } - end - - def comment_on_line(line_holder, line) - line[:num].find(comment_button_class).trigger 'click' - expect(line_holder).to have_xpath notes_holder_input_xpath - - notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_input[:class].include? notes_holder_input_class).to be true - - notes_holder_input.fill_in 'note[note]', with: test_note_comment - click_button 'Comment' - end - - def assert_comment_persistence(line_holder) - expect(line_holder).to have_xpath notes_holder_input_xpath - - notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_saved[:class].include? notes_holder_input_class).to be false - expect(notes_holder_saved).to have_content test_note_comment - end - end end -- cgit v1.2.1 From 20308867dd8832f3e7ae2cc87334c075ce8c5400 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Wed, 10 Aug 2016 11:27:02 +0100 Subject: Review changes --- spec/features/merge_requests/diff_notes_spec.rb | 66 +++++++++---------------- 1 file changed, 23 insertions(+), 43 deletions(-) diff --git a/spec/features/merge_requests/diff_notes_spec.rb b/spec/features/merge_requests/diff_notes_spec.rb index 8b01601b004..12e89742b79 100644 --- a/spec/features/merge_requests/diff_notes_spec.rb +++ b/spec/features/merge_requests/diff_notes_spec.rb @@ -22,129 +22,109 @@ feature 'Diff notes', js: true, feature: true do end context 'with an old line on the left and no line on the right' do - let(:line_holder) { find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..') } - it 'should allow commenting on the left side' do - should_allow_commenting line_holder, 'left' + should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'left') end it 'should not allow commenting on the right side' do - should_not_allow_commenting line_holder, 'right' + should_not_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'right') end end context 'with no line on the left and a new line on the right' do - let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..') } - it 'should not allow commenting on the left side' do - should_not_allow_commenting line_holder, 'left' + should_not_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'left') end it 'should allow commenting on the right side' do - should_allow_commenting line_holder, 'right' + should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'right') end end context 'with an old line on the left and a new line on the right' do - let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..') } - it 'should allow commenting on the left side' do - should_allow_commenting line_holder, 'left' + should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'left') end it 'should allow commenting on the right side' do - should_allow_commenting line_holder, 'right' + should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'right') end end context 'with an unchanged line on the left and an unchanged line on the right' do - let(:line_holder) { first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..') } - it 'should allow commenting on the left side' do - should_allow_commenting line_holder, 'left' + should_allow_commenting(first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..'), 'left') end it 'should allow commenting on the right side' do - should_allow_commenting line_holder, 'right' + should_allow_commenting(first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..'), 'right') end end context 'with a match line' do - let(:line_holder) { first('.match').find(:xpath, '..') } - it 'should not allow commenting on the left side' do - should_not_allow_commenting line_holder, 'left' + should_not_allow_commenting(first('.match').find(:xpath, '..'), 'left') end it 'should not allow commenting on the right side' do - should_not_allow_commenting line_holder, 'right' + should_not_allow_commenting(first('.match').find(:xpath, '..'), 'right') end end end context 'when hovering over the inline view diff file' do - let(:comment_button_class) { '.add-diff-note' } - - before(:each) do + before do visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) click_link 'Inline' end context 'with a new line' do - let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]') } - it 'should allow commenting' do - should_allow_commenting line_holder + should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) end end context 'with an old line' do - let(:line_holder) { find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]') } - it 'should allow commenting' do - should_allow_commenting line_holder + should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) end end context 'with an unchanged line' do - let(:line_holder) { find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]') } - it 'should allow commenting' do - should_allow_commenting line_holder + should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) end end context 'with a match line' do - let(:line_holder) { first('.match') } - it 'should not allow commenting' do - should_not_allow_commenting line_holder + should_not_allow_commenting(first('.match')) end end end def should_allow_commenting(line_holder, diff_side = nil) - line = get_line_components line_holder, diff_side + line = get_line_components(line_holder, diff_side) line[:content].hover expect(line[:num]).to have_css comment_button_class - comment_on_line line_holder, line + comment_on_line(line_holder, line) wait_for_ajax - assert_comment_persistence line_holder + assert_comment_persistence(line_holder) end def should_not_allow_commenting(line_holder, diff_side = nil) - line = get_line_components line_holder, diff_side + line = get_line_components(line_holder, diff_side) line[:content].hover expect(line[:num]).not_to have_css comment_button_class end def get_line_components(line_holder, diff_side = nil) if diff_side.nil? - get_inline_line_components line_holder + get_inline_line_components(line_holder) else - get_parallel_line_components line_holder, diff_side + get_parallel_line_components(line_holder, diff_side) end end @@ -162,7 +142,7 @@ feature 'Diff notes', js: true, feature: true do expect(line_holder).to have_xpath notes_holder_input_xpath notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_input[:class].include? notes_holder_input_class).to be true + expect(notes_holder_input[:class]).to include(notes_holder_input_class) notes_holder_input.fill_in 'note[note]', with: test_note_comment click_button 'Comment' @@ -172,7 +152,7 @@ feature 'Diff notes', js: true, feature: true do expect(line_holder).to have_xpath notes_holder_input_xpath notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_saved[:class].include? notes_holder_input_class).to be false + expect(notes_holder_saved[:class]).not_to include(notes_holder_input_class) expect(notes_holder_saved).to have_content test_note_comment end end -- cgit v1.2.1 From 91cd5ae71cbede8f4a05a61d98c4e252cdbfe8c7 Mon Sep 17 00:00:00 2001 From: Chris Peressini Date: Thu, 11 Aug 2016 10:11:17 +0000 Subject: Add reference to product map. Remove line that says the GitLab logo and user picture are in the sidebar. --- doc/development/ui_guide.md | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/doc/development/ui_guide.md b/doc/development/ui_guide.md index 3a8c823e026..2d1d504202c 100644 --- a/doc/development/ui_guide.md +++ b/doc/development/ui_guide.md @@ -15,11 +15,14 @@ repository and maintained by GitLab UX designers. ## Navigation GitLab's layout contains 2 sections: the left sidebar and the content. The left sidebar contains a static navigation menu. -This menu will be visible regardless of what page you visit. The left sidebar also contains the GitLab logo -and the current user's profile picture. The content section contains a header and the content itself. -The header describes the current GitLab page and what navigation is -available to user in this area. Depending on the area (project, group, profile setting) the header name and navigation may change. For example when user visits one of the -project pages the header will contain a project name and navigation for that project. When the user visits a group page it will contain a group name and navigation related to this group. +This menu will be visible regardless of what page you visit. +The content section contains a header and the content itself. The header describes the current GitLab page and what navigation is +available to the user in this area. Depending on the area (project, group, profile setting) the header name and navigation may change. For example, when the user visits one of the +project pages the header will contain the project's name and navigation for that project. When the user visits a group page it will contain the group's name and navigation related to this group. + +You can see a visual representation of the navigation in GitLab in the GitLab Product Map, which is located in the [Design Repository](gitlab-map-graffle) +along with [PDF](gitlab-map-pdf) and [PNG](gitlab-map-png) exports. + ### Adding new tab to header navigation @@ -99,3 +102,6 @@ Do not use both green and blue button in one form. display counts in the UI. [number_with_delimiter]: http://api.rubyonrails.org/classes/ActionView/Helpers/NumberHelper.html#method-i-number_with_delimiter +[gitlab-map-graffle]: https://gitlab.com/gitlab-org/gitlab-design/blob/master/production/resources/gitlab-map.graffle +[gitlab-map-pdf]: https://gitlab.com/gitlab-org/gitlab-design/raw/master/production/resources/gitlab-map.pdf +[gitlab-map-png]: https://gitlab.com/gitlab-org/gitlab-design/raw/master/production/resources/gitlab-map.png \ No newline at end of file -- cgit v1.2.1 From efab1677a7a2959a28a09393a2b6519072005534 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 11 Aug 2016 11:03:26 +0200 Subject: Fix attribute inclusion in import/export config ignored in some cases --- CHANGELOG | 3 +++ lib/gitlab/import_export/json_hash_builder.rb | 9 +++------ spec/lib/gitlab/import_export/reader_spec.rb | 3 ++- spec/support/import_export/import_export.yml | 4 ++++ 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 5aa5cbec279..b8f06f4c445 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -90,6 +90,9 @@ v 8.11.0 (unreleased) - Fix importing GitLab projects with an invalid MR source project - Sort folders with submodules in Files view !5521 +v 8.10.6 (unreleased) + - Fix import/export configuration missing some included attributes + v 8.10.5 - Add a data migration to fix some missing timestamps in the members table. !5670 - Revert the "Defend against 'Host' header injection" change in the source NGINX templates. !5706 diff --git a/lib/gitlab/import_export/json_hash_builder.rb b/lib/gitlab/import_export/json_hash_builder.rb index 008300bde45..b7db0e7a06a 100644 --- a/lib/gitlab/import_export/json_hash_builder.rb +++ b/lib/gitlab/import_export/json_hash_builder.rb @@ -57,10 +57,7 @@ module Gitlab # +value+ existing model to be included in the hash # +json_config_hash+ the original hash containing the root model def create_model_value(current_key, value, json_config_hash) - parsed_hash = { include: value } - parse_hash(value, parsed_hash) - - json_config_hash[current_key] = parsed_hash + json_config_hash[current_key] = parse_hash(value, { include: value }) end # Calls attributes finder to parse the hash and add any attributes to it @@ -69,8 +66,8 @@ module Gitlab # +parsed_hash+ the original hash def parse_hash(value, parsed_hash) @attributes_finder.parse(value) do |hash| - parsed_hash = { include: hash_or_merge(value, hash) } - end + { include: hash_or_merge(value, hash) } + end || parsed_hash end # Adds new model configuration to an existing hash with key +current_key+ diff --git a/spec/lib/gitlab/import_export/reader_spec.rb b/spec/lib/gitlab/import_export/reader_spec.rb index b76e14deca1..b6dec41d218 100644 --- a/spec/lib/gitlab/import_export/reader_spec.rb +++ b/spec/lib/gitlab/import_export/reader_spec.rb @@ -12,7 +12,8 @@ describe Gitlab::ImportExport::Reader, lib: true do except: [:iid], include: [:merge_request_diff, :merge_request_test] } }, - { commit_statuses: { include: :commit } }] + { commit_statuses: { include: :commit } }, + { project_members: { include: { user: { only: [:email] } } } }] } end diff --git a/spec/support/import_export/import_export.yml b/spec/support/import_export/import_export.yml index 3ceec506401..17136dee000 100644 --- a/spec/support/import_export/import_export.yml +++ b/spec/support/import_export/import_export.yml @@ -7,6 +7,8 @@ project_tree: - :merge_request_test - commit_statuses: - :commit + - project_members: + - :user included_attributes: project: @@ -14,6 +16,8 @@ included_attributes: - :path merge_requests: - :id + user: + - :email excluded_attributes: merge_requests: -- cgit v1.2.1 From 5f7394070f92acb2b7858b792034b30102fe1d9b Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 11 Aug 2016 14:22:21 +0200 Subject: Added documentation on adding database indexes --- doc/development/README.md | 4 + doc/development/adding_database_indexes.md | 123 +++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 doc/development/adding_database_indexes.md diff --git a/doc/development/README.md b/doc/development/README.md index bf67b5d8dff..57f37da6f80 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -30,7 +30,11 @@ - [Rake tasks](rake_tasks.md) for development - [Shell commands](shell_commands.md) in the GitLab codebase - [Sidekiq debugging](sidekiq_debugging.md) + +## Databases + - [What requires downtime?](what_requires_downtime.md) +- [Adding database indexes](adding_database_indexes.md) ## Compliance diff --git a/doc/development/adding_database_indexes.md b/doc/development/adding_database_indexes.md new file mode 100644 index 00000000000..ea6f14da3b9 --- /dev/null +++ b/doc/development/adding_database_indexes.md @@ -0,0 +1,123 @@ +# Adding Database Indexes + +Indexes can be used to speed up database queries, but when should you add a new +index? Traditionally the answer to this question has been to add an index for +every column used for filtering or joining data. For example, consider the +following query: + +```sql +SELECT * +FROM projects +WHERE user_id = 2; +``` + +Here we are filtering by the `user_id` column and as such a developer may decide +to index this column. + +While in certain cases indexing columns using the above approach may make sense +it can actually have a negative impact. Whenever you write data to a table any +existing indexes need to be updated. The more indexes there are the slower this +can potentially become. Indexes can also take up quite some disk space depending +on the amount of data indexed and the index type. For example, PostgreSQL offers +"GIN" indexes which can be used to index certain data types that can not be +indexed by regular btree indexes. These indexes however generally take up more +data and are slower to update compared to btree indexes. + +Because of all this one should not blindly add a new index for every column used +to filter data by. Instead one should ask themselves the following questions: + +1. Can I write my query in such a way that it re-uses as many existing indexes + as possible? +2. Is the data going to be large enough that using an index will actually be + faster than just iterating over the rows in the table? +3. Is the overhead of maintaining the index worth the reduction in query + timings? + +We'll explore every question in detail below. + +## Re-using Queries + +The first step is to make sure your query re-uses as many existing indexes as +possible. For example, consider the following query: + +```sql +SELECT * +FROM todos +WHERE user_id = 123 +AND state = 'open'; +``` + +Now imagine we already have an index on the `user_id` column but not on the +`state` column. One may think this query will perform badly due to `state` being +unindexed. In reality the query may perform just fine given the index on +`user_id` can filter out enough rows. + +The best way to determine if indexes are re-used is to run your query using +`EXPLAIN ANALYZE`. Depending on any extra tables that may be joined and +other columns being used for filtering you may find an extra index is not going +to make much (if any) difference. On the other hand you may determine that the +index _may_ make a difference. + +In short: + +1. Try to write your query in such a way that it re-uses as many existing + indexes as possible. +2. Run the query using `EXPLAIN ANALYZE` and study the output to find the most + ideal query. + +## Data Size + +A database may decide not to use an index despite it existing in case a regular +sequence scan (= simply iterating over all existing rows) is faster. This is +especially the case for small tables. + +If a table is expected to grow in size and you expect your query has to filter +out a lot of rows you may want to consider adding an index. If the table size is +very small (e.g. only a handful of rows) or any existing indexes filter out +enough rows you may _not_ want to add a new index. + +## Maintenance Overhead + +Indexes have to be updated on every table write. In case of PostgreSQL _all_ +existing indexes will be updated whenever data is written to a table. As a +result of this having many indexes on the same table will slow down writes. + +Because of this one should ask themselves: is the reduction in query performance +worth the overhead of maintaining an extra index? + +If adding an index reduces SELECT timings by 5 milliseconds but increases +INSERT/UPDATE/DELETE timings by 10 milliseconds then the index may not be worth +it. On the other hand, if SELECT timings are reduced but INSERT/UPDATE/DELETE +timings are not affected you may want to add the index after all. + +## Finding Unused Indexes + +To see which indexes are unused you can run the following query: + +```sql +SELECT relname as table_name, indexrelname as index_name, idx_scan, idx_tup_read, idx_tup_fetch, pg_size_pretty(pg_relation_size(indexrelname::regclass)) +FROM pg_stat_all_indexes +WHERE schemaname = 'public' +AND idx_scan = 0 +AND idx_tup_read = 0 +AND idx_tup_fetch = 0 +ORDER BY pg_relation_size(indexrelname::regclass) desc; +``` + +This query outputs a list containing all indexes that are never used and sorts +them by indexes sizes in descending order. This query can be useful to +determine if any previously indexes are useful after all. More information on +the meaning of the various columns can be found at +. + +Because the output of this query relies on the actual usage of your database it +may be affected by factors such as (but not limited to): + +* Certain queries never being executed, thus not being able to use certain + indexes. +* Certain tables having little data, resulting in PostgreSQL using sequence + scans instead of index scans. + +In other words, this data is only reliable for a frequently used database with +plenty of data and with as many GitLab features enabled (and being used) as +possible. -- cgit v1.2.1 From 30f9596c612abc19dd060fa3a8e8ae3d92001d45 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 11 Aug 2016 16:59:37 +0200 Subject: Fix permissions check in controller, added relevant spec and updated docs --- .../import/gitlab_projects_controller.rb | 5 ++ app/views/projects/new.html.haml | 2 +- doc/user/project/settings/import_export.md | 3 +- features/dashboard/new_project.feature | 2 +- features/steps/dashboard/new_project.rb | 3 +- .../projects/import_export/import_file_spec.rb | 98 +++++++++++++--------- 6 files changed, 69 insertions(+), 44 deletions(-) diff --git a/app/controllers/import/gitlab_projects_controller.rb b/app/controllers/import/gitlab_projects_controller.rb index 3ec173abcdb..7d0eff37635 100644 --- a/app/controllers/import/gitlab_projects_controller.rb +++ b/app/controllers/import/gitlab_projects_controller.rb @@ -1,5 +1,6 @@ class Import::GitlabProjectsController < Import::BaseController before_action :verify_gitlab_project_import_enabled + before_action :authenticate_admin! def new @namespace_id = project_params[:namespace_id] @@ -47,4 +48,8 @@ class Import::GitlabProjectsController < Import::BaseController :path, :namespace_id, :file ) end + + def authenticate_admin! + render_404 unless current_user.is_admin? + end end diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml index adcc984f506..ea4898f2107 100644 --- a/app/views/projects/new.html.haml +++ b/app/views/projects/new.html.haml @@ -77,7 +77,7 @@ = link_to "#", class: 'btn js-toggle-button import_git' do = icon('git', text: 'Repo by URL') %div{ class: 'import_gitlab_project' } - - if gitlab_project_import_enabled? + - if gitlab_project_import_enabled? && current_user.is_admin? = link_to new_import_gitlab_project_path, class: 'btn btn_import_gitlab_project project-submit' do = icon('gitlab', text: 'GitLab export') diff --git a/doc/user/project/settings/import_export.md b/doc/user/project/settings/import_export.md index 2513def49a4..08ff89ce6ae 100644 --- a/doc/user/project/settings/import_export.md +++ b/doc/user/project/settings/import_export.md @@ -7,8 +7,7 @@ > than that of the exporter. > - For existing installations, the project import option has to be enabled in > application settings (`/admin/application_settings`) under 'Import sources'. -> Ask your administrator if you don't see the **GitLab export** button when -> creating a new project. +> You will have to be an administrator to enable and use the import functionality. > - You can find some useful raketasks if you are an administrator in the > [import_export](../../../administration/raketasks/project_import_export.md) > raketask. diff --git a/features/dashboard/new_project.feature b/features/dashboard/new_project.feature index 8ddafb6a7ac..046e2815d4e 100644 --- a/features/dashboard/new_project.feature +++ b/features/dashboard/new_project.feature @@ -9,7 +9,7 @@ Background: @javascript Scenario: I should see New Projects page Then I see "New Project" page - Then I see all possible import optios + Then I see all possible import options @javascript Scenario: I should see instructions on how to import from Git URL diff --git a/features/steps/dashboard/new_project.rb b/features/steps/dashboard/new_project.rb index 727a6a71373..f2b44734601 100644 --- a/features/steps/dashboard/new_project.rb +++ b/features/steps/dashboard/new_project.rb @@ -14,14 +14,13 @@ class Spinach::Features::NewProject < Spinach::FeatureSteps expect(page).to have_content('Project name') end - step 'I see all possible import optios' do + step 'I see all possible import options' do expect(page).to have_link('GitHub') expect(page).to have_link('Bitbucket') expect(page).to have_link('GitLab.com') expect(page).to have_link('Gitorious.org') expect(page).to have_link('Google Code') expect(page).to have_link('Repo by URL') - expect(page).to have_link('GitLab export') end step 'I click on "Import project from GitHub"' do diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index 7835e1678ad..f707ccf4e93 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -3,8 +3,9 @@ require 'spec_helper' feature 'project import', feature: true, js: true do include Select2Helper - let(:user) { create(:admin) } - let!(:namespace) { create(:namespace, name: "asd", owner: user) } + let(:admin) { create(:admin) } + let(:normal_user) { create(:user) } + let!(:namespace) { create(:namespace, name: "asd", owner: admin) } let(:file) { File.join(Rails.root, 'spec', 'features', 'projects', 'import_export', 'test_project_export.tar.gz') } let(:export_path) { "#{Dir::tmpdir}/import_file_spec" } let(:project) { Project.last } @@ -12,66 +13,87 @@ feature 'project import', feature: true, js: true do background do allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) - login_as(user) end after(:each) do FileUtils.rm_rf(export_path, secure: true) end - scenario 'user imports an exported project successfully' do - expect(Project.all.count).to be_zero + context 'admin user' do + before do + login_as(admin) + end - visit new_project_path + scenario 'user imports an exported project successfully' do + expect(Project.all.count).to be_zero - select2('2', from: '#project_namespace_id') - fill_in :project_path, with: 'test-project-path', visible: true - click_link 'GitLab export' + visit new_project_path - expect(page).to have_content('GitLab project export') - expect(URI.parse(current_url).query).to eq('namespace_id=2&path=test-project-path') + select2('2', from: '#project_namespace_id') + fill_in :project_path, with: 'test-project-path', visible: true + click_link 'GitLab export' - attach_file('file', file) + expect(page).to have_content('GitLab project export') + expect(URI.parse(current_url).query).to eq('namespace_id=2&path=test-project-path') - click_on 'Import project' # import starts + attach_file('file', file) - expect(project).not_to be_nil - expect(project.issues).not_to be_empty - expect(project.merge_requests).not_to be_empty - expect(project_hook).to exist - expect(wiki_exists?).to be true - expect(project.import_status).to eq('finished') - end + click_on 'Import project' # import starts + + expect(project).not_to be_nil + expect(project.issues).not_to be_empty + expect(project.merge_requests).not_to be_empty + expect(project_hook).to exist + expect(wiki_exists?).to be true + expect(project.import_status).to eq('finished') + end - scenario 'invalid project' do - project = create(:project, namespace_id: 2) + scenario 'invalid project' do + project = create(:project, namespace_id: 2) - visit new_project_path + visit new_project_path - select2('2', from: '#project_namespace_id') - fill_in :project_path, with: project.name, visible: true - click_link 'GitLab export' + select2('2', from: '#project_namespace_id') + fill_in :project_path, with: project.name, visible: true + click_link 'GitLab export' - attach_file('file', file) - click_on 'Import project' + attach_file('file', file) + click_on 'Import project' - page.within('.flash-container') do - expect(page).to have_content('Project could not be imported') + page.within('.flash-container') do + expect(page).to have_content('Project could not be imported') + end + end + + scenario 'project with no name' do + create(:project, namespace_id: 2) + + visit new_project_path + + select2('2', from: '#project_namespace_id') + + # click on disabled element + find(:link, 'GitLab export').trigger('click') + + page.within('.flash-container') do + expect(page).to have_content('Please enter path and name') + end end end - scenario 'project with no name' do - create(:project, namespace_id: 2) + context 'normal user' do + before do + login_as(normal_user) + end - visit new_project_path + scenario 'non-admin user is not allowed to import a project' do + expect(Project.all.count).to be_zero - select2('2', from: '#project_namespace_id') + visit new_project_path - # click on disabled element - find(:link, 'GitLab export').trigger('click') + fill_in :project_path, with: 'test-project-path', visible: true - page.within('.flash-container') do - expect(page).to have_content('Please enter path and name') + expect(page).not_to have_content('GitLab export') end end -- cgit v1.2.1 From c42f5f8b56a919473d9adceaf84c9ef77179c5cb Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 11 Aug 2016 21:42:34 +0200 Subject: refactor parse_hash based on feedback --- lib/gitlab/import_export/json_hash_builder.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/import_export/json_hash_builder.rb b/lib/gitlab/import_export/json_hash_builder.rb index b7db0e7a06a..0cc10f40087 100644 --- a/lib/gitlab/import_export/json_hash_builder.rb +++ b/lib/gitlab/import_export/json_hash_builder.rb @@ -57,17 +57,17 @@ module Gitlab # +value+ existing model to be included in the hash # +json_config_hash+ the original hash containing the root model def create_model_value(current_key, value, json_config_hash) - json_config_hash[current_key] = parse_hash(value, { include: value }) + json_config_hash[current_key] = parse_hash(value) || { include: value } end # Calls attributes finder to parse the hash and add any attributes to it # # +value+ existing model to be included in the hash # +parsed_hash+ the original hash - def parse_hash(value, parsed_hash) + def parse_hash(value) @attributes_finder.parse(value) do |hash| { include: hash_or_merge(value, hash) } - end || parsed_hash + end end # Adds new model configuration to an existing hash with key +current_key+ -- cgit v1.2.1 From ade0c2c8922c0838ba85cf69419cbb109453d6b2 Mon Sep 17 00:00:00 2001 From: Frank West Date: Mon, 8 Aug 2016 03:29:23 +0000 Subject: Prevents accidental overwrites of commits from UI Currently when a user performs an update of a file through the UI and there has already been a change committed to the file the previous commits will be overwritten without a check to see if the file has been changed. This commit uses the last commit sha at the time the user starts editing the file and compares it with the current sha of the file being edited to ensure they are the same before committing the file. If the shas do not match we throw an exception preventing the commit from the commit from occurring. Fixes #5857 --- CHANGELOG | 1 + app/controllers/projects/blob_controller.rb | 14 +++- app/services/files/base_service.rb | 1 + app/services/files/update_service.rb | 23 ++++++ app/views/projects/blob/edit.html.haml | 9 ++- .../features/projects/files/editing_a_file_spec.rb | 34 +++++++++ spec/services/files/update_service_spec.rb | 84 ++++++++++++++++++++++ 7 files changed, 162 insertions(+), 4 deletions(-) create mode 100644 spec/features/projects/files/editing_a_file_spec.rb create mode 100644 spec/services/files/update_service_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 6e096b480c0..d424d6aebc6 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -108,6 +108,7 @@ v 8.11.0 (unreleased) - Sort folders with submodules in Files view !5521 - Each `File::exists?` replaced to `File::exist?` because of deprecate since ruby version 2.2.0 - Add auto-completition in pipeline (Katarzyna Kobierska Ula Budziszewska) + - Ensure file editing in UI does not overwrite commited changes without warning user v 8.10.5 - Add a data migration to fix some missing timestamps in the members table. !5670 diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 19d051720e9..cdf9a04bacf 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -17,6 +17,7 @@ class Projects::BlobController < Projects::ApplicationController before_action :require_branch_head, only: [:edit, :update] before_action :editor_variables, except: [:show, :preview, :diff] before_action :validate_diff_params, only: :diff + before_action :set_last_commit_sha, only: [:edit, :update] def new commit unless @repository.empty? @@ -33,7 +34,6 @@ class Projects::BlobController < Projects::ApplicationController end def edit - @last_commit = Gitlab::Git::Commit.last_for_path(@repository, @ref, @path).sha blob.load_all_data!(@repository) end @@ -55,6 +55,10 @@ class Projects::BlobController < Projects::ApplicationController create_commit(Files::UpdateService, success_path: after_edit_path, failure_view: :edit, failure_path: namespace_project_blob_path(@project.namespace, @project, @id)) + + rescue Files::UpdateService::FileChangedError + @conflict = true + render :edit end def preview @@ -152,7 +156,8 @@ class Projects::BlobController < Projects::ApplicationController file_path: @file_path, commit_message: params[:commit_message], file_content: params[:content], - file_content_encoding: params[:encoding] + file_content_encoding: params[:encoding], + last_commit_sha: params[:last_commit_sha] } end @@ -161,4 +166,9 @@ class Projects::BlobController < Projects::ApplicationController render nothing: true end end + + def set_last_commit_sha + @last_commit_sha = Gitlab::Git::Commit. + last_for_path(@repository, @ref, @path).sha + end end diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index c4a206f785e..ea94818713b 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -15,6 +15,7 @@ module Files else params[:file_content] end + @last_commit_sha = params[:last_commit_sha] # Validate parameters validate diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index 8d2b5083179..4fc3b640799 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -2,11 +2,34 @@ require_relative "base_service" module Files class UpdateService < Files::BaseService + class FileChangedError < StandardError; end + def commit repository.update_file(current_user, @file_path, @file_content, branch: @target_branch, previous_path: @previous_path, message: @commit_message) end + + private + + def validate + super + + if file_has_changed? + raise FileChangedError.new("You are attempting to update a file that has changed since you started editing it.") + end + end + + def file_has_changed? + return false unless @last_commit_sha && last_commit + + @last_commit_sha != last_commit.sha + end + + def last_commit + @last_commit ||= Gitlab::Git::Commit. + last_for_path(@source_project.repository, @source_branch, @file_path) + end end end diff --git a/app/views/projects/blob/edit.html.haml b/app/views/projects/blob/edit.html.haml index b1c9895f43e..7b0621f9401 100644 --- a/app/views/projects/blob/edit.html.haml +++ b/app/views/projects/blob/edit.html.haml @@ -1,5 +1,11 @@ - page_title "Edit", @blob.path, @ref +- if @conflict + .alert.alert-danger + Someone edited the file the same time you did. Please check out + = link_to "the file", namespace_project_blob_path(@project.namespace, @project, tree_join(@target_branch, @file_path)), target: "_blank" + and make sure your changes will not unintentionally remove theirs. + .file-editor %ul.nav-links.no-bottom.js-edit-mode %li.active @@ -13,8 +19,7 @@ = form_tag(namespace_project_update_blob_path(@project.namespace, @project, @id), method: :put, class: 'form-horizontal js-quick-submit js-requires-input js-edit-blob-form') do = render 'projects/blob/editor', ref: @ref, path: @path, blob_data: @blob.data = render 'shared/new_commit_form', placeholder: "Update #{@blob.name}" - - = hidden_field_tag 'last_commit', @last_commit + = hidden_field_tag 'last_commit_sha', @last_commit_sha = hidden_field_tag 'content', '', id: "file-content" = hidden_field_tag 'from_merge_request_id', params[:from_merge_request_id] = render 'projects/commit_button', ref: @ref, cancel_path: namespace_project_blob_path(@project.namespace, @project, @id) diff --git a/spec/features/projects/files/editing_a_file_spec.rb b/spec/features/projects/files/editing_a_file_spec.rb new file mode 100644 index 00000000000..fe047e00409 --- /dev/null +++ b/spec/features/projects/files/editing_a_file_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +feature 'User wants to edit a file', feature: true do + include WaitForAjax + + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:commit_params) do + { + source_branch: project.default_branch, + target_branch: project.default_branch, + commit_message: "Committing First Update", + file_path: ".gitignore", + file_content: "First Update", + last_commit_sha: Gitlab::Git::Commit.last_for_path(project.repository, project.default_branch, + ".gitignore").sha + } + end + + background do + project.team << [user, :master] + login_as user + visit namespace_project_edit_blob_path(project.namespace, project, + File.join(project.default_branch, '.gitignore')) + end + + scenario 'file has been updated since the user opened the edit page' do + Files::UpdateService.new(project, user, commit_params).execute + + click_button 'Commit Changes' + + expect(page).to have_content 'Someone edited the file the same time you did.' + end +end diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb new file mode 100644 index 00000000000..d019e50649f --- /dev/null +++ b/spec/services/files/update_service_spec.rb @@ -0,0 +1,84 @@ +require "spec_helper" + +describe Files::UpdateService do + subject { described_class.new(project, user, commit_params) } + + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:file_path) { 'files/ruby/popen.rb' } + let(:new_contents) { "New Content" } + let(:commit_params) do + { + file_path: file_path, + commit_message: "Update File", + file_content: new_contents, + file_content_encoding: "text", + last_commit_sha: last_commit_sha, + source_project: project, + source_branch: project.default_branch, + target_branch: project.default_branch, + } + end + + before do + project.team << [user, :master] + end + + describe "#execute" do + context "when the file's last commit sha does not match the supplied last_commit_sha" do + let(:last_commit_sha) { "foo" } + + it "returns a hash with the correct error message and a :error status " do + expect { subject.execute }. + to raise_error(Files::UpdateService::FileChangedError, + "You are attempting to update a file that has changed since you started editing it.") + end + end + + context "when the file's last commit sha does match the supplied last_commit_sha" do + let(:last_commit_sha) { Gitlab::Git::Commit.last_for_path(project.repository, project.default_branch, file_path).sha } + + it "returns a hash with the :success status " do + results = subject.execute + + expect(results).to match({ status: :success }) + end + + it "updates the file with the new contents" do + subject.execute + + results = project.repository.blob_at_branch(project.default_branch, file_path) + + expect(results.data).to eq(new_contents) + end + end + + context "when the last_commit_sha is not supplied" do + let(:commit_params) do + { + file_path: file_path, + commit_message: "Update File", + file_content: new_contents, + file_content_encoding: "text", + source_project: project, + source_branch: project.default_branch, + target_branch: project.default_branch, + } + end + + it "returns a hash with the :success status " do + results = subject.execute + + expect(results).to match({ status: :success }) + end + + it "updates the file with the new contents" do + subject.execute + + results = project.repository.blob_at_branch(project.default_branch, file_path) + + expect(results.data).to eq(new_contents) + end + end + end +end -- cgit v1.2.1 From 95419679f23f0628d1885dd9656cc159e9d55ea9 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Wed, 27 Jul 2016 19:03:06 -0500 Subject: Lay the ground works to submit information to Akismet - New concern `AkismetSubmittable` to allow issues and other `Spammable` models to be submitted to Akismet. - New model `UserAgentDetail` to store information needed for Akismet. - Services needed for their creation and tests. --- app/models/concerns/akismet_submittable.rb | 15 +++++++++++++++ app/models/issue.rb | 1 + app/models/user_agent_detail.rb | 16 ++++++++++++++++ app/services/issues/create_service.rb | 5 +++++ app/services/user_agent_detail_service.rb | 12 ++++++++++++ .../20160727163552_create_user_agent_details.rb | 12 ++++++++++++ db/schema.rb | 9 +++++++++ .../controllers/projects/issues_controller_spec.rb | 20 ++++++++++++++++++++ spec/factories/user_agent_details.rb | 10 ++++++++++ spec/models/user_agent_detail_spec.rb | 22 ++++++++++++++++++++++ 10 files changed, 122 insertions(+) create mode 100644 app/models/concerns/akismet_submittable.rb create mode 100644 app/models/user_agent_detail.rb create mode 100644 app/services/user_agent_detail_service.rb create mode 100644 db/migrate/20160727163552_create_user_agent_details.rb create mode 100644 spec/factories/user_agent_details.rb create mode 100644 spec/models/user_agent_detail_spec.rb diff --git a/app/models/concerns/akismet_submittable.rb b/app/models/concerns/akismet_submittable.rb new file mode 100644 index 00000000000..17821688941 --- /dev/null +++ b/app/models/concerns/akismet_submittable.rb @@ -0,0 +1,15 @@ +module AkismetSubmittable + extend ActiveSupport::Concern + + included do + has_one :user_agent_detail, as: :subject + end + + def can_be_submitted? + if user_agent_detail + user_agent_detail.submittable? + else + false + end + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index d62ffb21467..6c2635498e5 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -8,6 +8,7 @@ class Issue < ActiveRecord::Base include Taskable include Spammable include FasterCacheKeys + include AkismetSubmittable DueDateStruct = Struct.new(:title, :name).freeze NoDueDate = DueDateStruct.new('No Due Date', '0').freeze diff --git a/app/models/user_agent_detail.rb b/app/models/user_agent_detail.rb new file mode 100644 index 00000000000..6d76dff20e3 --- /dev/null +++ b/app/models/user_agent_detail.rb @@ -0,0 +1,16 @@ +class UserAgentDetail < ActiveRecord::Base + belongs_to :subject, polymorphic: true + + validates :user_agent, + presence: true + validates :ip_address, + presence: true + validates :subject_id, + presence: true + validates :subject_type, + presence: true + + def submittable? + user_agent.present? && ip_address.present? + end +end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 5e2de2ccf64..8e9d74103c7 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -15,6 +15,7 @@ module Issues notification_service.new_issue(issue, current_user) todo_service.new_issue(issue, current_user) event_service.open_issue(issue, current_user) + user_agent_detail_service(issue, request).create issue.create_cross_references!(current_user) execute_hooks(issue, 'open') end @@ -27,5 +28,9 @@ module Issues def spam_check_service SpamCheckService.new(project, current_user, params) end + + def user_agent_detail_service(issue, request) + UserAgentDetailService.new(issue, request) + end end end diff --git a/app/services/user_agent_detail_service.rb b/app/services/user_agent_detail_service.rb new file mode 100644 index 00000000000..dd995955be3 --- /dev/null +++ b/app/services/user_agent_detail_service.rb @@ -0,0 +1,12 @@ +class UserAgentDetailService + attr_accessor :subject, :request + + def initialize(subject, request) + @subject, @request = subject, request + end + + def create + return unless request + subject.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s) + end +end diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb new file mode 100644 index 00000000000..05c21a476fa --- /dev/null +++ b/db/migrate/20160727163552_create_user_agent_details.rb @@ -0,0 +1,12 @@ +class CreateUserAgentDetails < ActiveRecord::Migration + def change + create_table :user_agent_details do |t| + t.string :user_agent, null: false + t.string :ip_address, null: false + t.integer :subject_id, null: false + t.string :subject_type, null: false + + t.timestamps null: false + end + end +end diff --git a/db/schema.rb b/db/schema.rb index f008a6bd7a7..2e5ffac5935 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -999,6 +999,15 @@ ActiveRecord::Schema.define(version: 20160810142633) do add_index "u2f_registrations", ["key_handle"], name: "index_u2f_registrations_on_key_handle", using: :btree add_index "u2f_registrations", ["user_id"], name: "index_u2f_registrations_on_user_id", using: :btree + create_table "user_agent_details", force: :cascade do |t| + t.string "user_agent", null: false + t.string "ip_address", null: false + t.integer "subject_id", null: false + t.string "subject_type", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "users", force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index b6a0276846c..4e39826d694 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -300,6 +300,26 @@ describe Projects::IssuesController do expect(spam_logs[0].title).to eq('Spam Title') end end + + context 'user agent details are saved' do + before do + request.env['action_dispatch.remote_ip'] = '127.0.0.1' + end + + def post_new_issue + sign_in(user) + project = create(:empty_project, :public) + post :create, { + namespace_id: project.namespace.to_param, + project_id: project.to_param, + issue: { title: 'Title', description: 'Description' } + } + end + + it 'creates a user agent detail' do + expect{ post_new_issue }.to change(UserAgentDetail, :count) + end + end end describe "DELETE #destroy" do diff --git a/spec/factories/user_agent_details.rb b/spec/factories/user_agent_details.rb new file mode 100644 index 00000000000..5fc40915911 --- /dev/null +++ b/spec/factories/user_agent_details.rb @@ -0,0 +1,10 @@ +FactoryGirl.define do + factory :user_agent_detail do + ip_address '127.0.0.1' + user_agent 'AppleWebKit/537.36' + + trait :on_issue do + association :subject, factory: :issue + end + end +end diff --git a/spec/models/user_agent_detail_spec.rb b/spec/models/user_agent_detail_spec.rb new file mode 100644 index 00000000000..8debcbbeba6 --- /dev/null +++ b/spec/models/user_agent_detail_spec.rb @@ -0,0 +1,22 @@ +require 'rails_helper' + +describe UserAgentDetail, type: :model do + describe '.submittable?' do + it 'should be submittable' do + detail = create(:user_agent_detail, :on_issue) + expect(detail.submittable?).to be_truthy + end + end + + describe '.valid?' do + it 'should be valid with a subject' do + detail = create(:user_agent_detail, :on_issue) + expect(detail).to be_valid + end + + it 'should not be valid without a subject' do + detail = build(:user_agent_detail) + expect(detail).not_to be_valid + end + end +end -- cgit v1.2.1 From 722fc84e3d4785fb3a9db5f1c7d2aabad22e8e01 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Thu, 28 Jul 2016 19:02:56 -0500 Subject: Complete refactor of the `Spammable` concern and tests: - Merged `AkismetSubmittable` into `Spammable` - Clean up `SpamCheckService` - Added tests for `Spammable` - Added submit (ham or spam) options to `AkismetHelper` --- app/models/concerns/akismet_submittable.rb | 15 -------- app/models/concerns/spammable.rb | 58 ++++++++++++++++++++++++++++-- app/models/issue.rb | 2 ++ app/services/issues/create_service.rb | 2 +- app/services/spam_check_service.rb | 22 +++++------- lib/api/issues.rb | 2 +- lib/gitlab/akismet_helper.rb | 34 ++++++++++++++++++ spec/factories/user_agent_details.rb | 2 ++ spec/models/concerns/spammable_spec.rb | 41 +++++++++++++++++++++ spec/models/user_agent_detail_spec.rb | 5 --- 10 files changed, 145 insertions(+), 38 deletions(-) delete mode 100644 app/models/concerns/akismet_submittable.rb create mode 100644 spec/models/concerns/spammable_spec.rb diff --git a/app/models/concerns/akismet_submittable.rb b/app/models/concerns/akismet_submittable.rb deleted file mode 100644 index 17821688941..00000000000 --- a/app/models/concerns/akismet_submittable.rb +++ /dev/null @@ -1,15 +0,0 @@ -module AkismetSubmittable - extend ActiveSupport::Concern - - included do - has_one :user_agent_detail, as: :subject - end - - def can_be_submitted? - if user_agent_detail - user_agent_detail.submittable? - else - false - end - end -end diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 3b8e6df2da9..bbf6a3e0be3 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -1,16 +1,70 @@ module Spammable extend ActiveSupport::Concern + include Gitlab::AkismetHelper + + module ClassMethods + def attr_spammable(*attrs) + attrs.each do |attr| + spammable_attrs << attr.to_s + end + end + end included do + has_one :user_agent_detail, as: :subject, dependent: :destroy attr_accessor :spam after_validation :check_for_spam, on: :create + + cattr_accessor :spammable_attrs, instance_accessor: false do + [] + end + end + + def can_be_submitted? + if user_agent_detail + user_agent_detail.submittable? + else + false + end + end + + def submit_ham + return unless akismet_enabled? && can_be_submitted? + ham!(user_agent_detail, spammable_text, creator) + end + + def submit_spam + return unless akismet_enabled? && can_be_submitted? + spam!(user_agent_detail, spammable_text, creator) + end + + def spam?(env, user) + is_spam?(env, user, spammable_text) end - def spam? + def spam_detected? @spam end def check_for_spam - self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam? + self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam_detected? + end + + private + + def spammable_text + result = [] + self.class.spammable_attrs.each do |entry| + result << self.send(entry) + end + result.reject(&:blank?).join("\n") + end + + def creator + if self.author_id + User.find(self.author_id) + elsif self.creator_id + User.find(self.creator_id) + end end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 6c2635498e5..5408e24b21c 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -37,6 +37,8 @@ class Issue < ActiveRecord::Base scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') } scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') } + attr_spammable :title, :description + state_machine :state, initial: :opened do event :close do transition [:reopened, :opened] => :closed diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 8e9d74103c7..d580834be83 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -8,7 +8,7 @@ module Issues issue = project.issues.new(params) issue.author = params[:author] || current_user - issue.spam = spam_check_service.execute(request, api) + issue.spam = spam_check_service.execute(request, api, issue) if issue.save issue.update_attributes(label_ids: label_params) diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb index 7c3e692bde9..7d6754546a8 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -1,23 +1,17 @@ class SpamCheckService < BaseService - include Gitlab::AkismetHelper + attr_accessor :request, :api, :subject - attr_accessor :request, :api + def execute(request, api, subject) + @request, @api, @subject = request, api, subject + return false unless request || subject.check_for_spam?(project) + return false unless subject.spam?(request.env, current_user) - def execute(request, api) - @request, @api = request, api - return false unless request || check_for_spam?(project) - return false unless is_spam?(request.env, current_user, text) - create_spam_log true end private - - def text - [params[:title], params[:description]].reject(&:blank?).join("\n") - end def spam_log_attrs { @@ -25,9 +19,9 @@ class SpamCheckService < BaseService project_id: project.id, title: params[:title], description: params[:description], - source_ip: client_ip(request.env), - user_agent: user_agent(request.env), - noteable_type: 'Issue', + source_ip: subject.client_ip(request.env), + user_agent: subject.user_agent(request.env), + noteable_type: subject.class.to_s, via_api: api } end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index c4d3134da6c..7bbfc137c2c 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -160,7 +160,7 @@ module API issue = ::Issues::CreateService.new(project, current_user, attrs.merge(request: request, api: true)).execute - if issue.spam? + if issue.spam_detected? render_api_error!({ error: 'Spam detected' }, 400) end diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb index 207736b59db..19e73820321 100644 --- a/lib/gitlab/akismet_helper.rb +++ b/lib/gitlab/akismet_helper.rb @@ -43,5 +43,39 @@ module Gitlab false end end + + def ham!(details, text, user) + client = akismet_client + + params = { + type: 'comment', + text: text, + author: user.name, + author_email: user.email + } + + begin + client.submit_ham(details.ip_address, details.user_agent, params) + rescue => e + Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") + end + end + + def spam!(details, text, user) + client = akismet_client + + params = { + type: 'comment', + text: text, + author: user.name, + author_email: user.email + } + + begin + client.submit_spam(details.ip_address, details.user_agent, params) + rescue => e + Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") + end + end end end diff --git a/spec/factories/user_agent_details.rb b/spec/factories/user_agent_details.rb index 5fc40915911..10de5dcb329 100644 --- a/spec/factories/user_agent_details.rb +++ b/spec/factories/user_agent_details.rb @@ -2,6 +2,8 @@ FactoryGirl.define do factory :user_agent_detail do ip_address '127.0.0.1' user_agent 'AppleWebKit/537.36' + subject_id 1 + subject_type 'Issue' trait :on_issue do association :subject, factory: :issue diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb new file mode 100644 index 00000000000..e61a6dcb69d --- /dev/null +++ b/spec/models/concerns/spammable_spec.rb @@ -0,0 +1,41 @@ +require 'spec_helper' + +describe Issue, 'Spammable' do + let(:issue) { create(:issue, description: 'Test Desc.') } + + describe 'Associations' do + it { is_expected.to have_one(:user_agent_detail).dependent(:destroy) } + end + + describe 'ClassMethods' do + it 'should return correct attr_spammable' do + expect(issue.send(:spammable_text)).to eq("#{issue.title}\n#{issue.description}") + end + end + + describe 'InstanceMethods' do + it 'should return the correct creator' do + expect(issue.send(:creator).id).to eq(issue.author_id) + end + + it 'should be invalid if spam' do + issue.spam = true + expect(issue.valid?).to be_truthy + end + + it 'should be submittable' do + create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s) + expect(issue.can_be_submitted?).to be_truthy + end + end + + describe 'AkismetMethods' do + before do + allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(check_for_spam?: true, is_spam?: true, ham!: nil, spam!: nil) + end + + it { expect(issue.spam?(:mock_env, :mock_user)).to be_truthy } + it { expect(issue.submit_spam).to be_nil } + it { expect(issue.submit_ham).to be_nil } + end +end diff --git a/spec/models/user_agent_detail_spec.rb b/spec/models/user_agent_detail_spec.rb index 8debcbbeba6..ba21161fc7f 100644 --- a/spec/models/user_agent_detail_spec.rb +++ b/spec/models/user_agent_detail_spec.rb @@ -13,10 +13,5 @@ describe UserAgentDetail, type: :model do detail = create(:user_agent_detail, :on_issue) expect(detail).to be_valid end - - it 'should not be valid without a subject' do - detail = build(:user_agent_detail) - expect(detail).not_to be_valid - end end end -- cgit v1.2.1 From 64ab2b3d9f10366249c03a6bcf5e8b1d20010d8f Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Fri, 29 Jul 2016 23:18:32 -0500 Subject: Refactored spam related code even further - Removed unnecessary column from `SpamLog` - Moved creation of SpamLogs out of its own service and into SpamCheckService - Simplified code in SpamCheckService. - Moved move spam related code into Spammable concern --- app/controllers/admin/spam_logs_controller.rb | 6 +++ app/models/concerns/spammable.rb | 44 ++++++++++++++-------- app/models/issue.rb | 13 +++++++ app/services/create_spam_log_service.rb | 13 ------- app/services/issues/create_service.rb | 34 ++++++++--------- app/services/spam_check_service.rb | 35 ++++++++--------- app/services/user_agent_detail_service.rb | 8 ++-- ...60729173930_remove_project_id_from_spam_logs.rb | 29 ++++++++++++++ db/schema.rb | 1 - lib/api/issues.rb | 2 +- lib/gitlab/akismet_helper.rb | 8 +--- .../controllers/projects/issues_controller_spec.rb | 2 +- spec/lib/gitlab/akismet_helper_spec.rb | 11 ------ spec/models/concerns/spammable_spec.rb | 21 +++++++++-- spec/requests/api/issues_spec.rb | 3 +- 15 files changed, 137 insertions(+), 93 deletions(-) delete mode 100644 app/services/create_spam_log_service.rb create mode 100644 db/migrate/20160729173930_remove_project_id_from_spam_logs.rb diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb index 3a2f0185315..bc6fce0ec4e 100644 --- a/app/controllers/admin/spam_logs_controller.rb +++ b/app/controllers/admin/spam_logs_controller.rb @@ -14,4 +14,10 @@ class Admin::SpamLogsController < Admin::ApplicationController head :ok end end + + def ham + spam_log = SpamLog.find(params[:id]) + + Gitlab::AkismetHelper.ham!(spam_log.source_ip, spam_log.user_agent, spam_log.description, spam_log.user) + end end diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index bbf6a3e0be3..5c75275b6e2 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -28,26 +28,42 @@ module Spammable end end - def submit_ham - return unless akismet_enabled? && can_be_submitted? - ham!(user_agent_detail, spammable_text, creator) - end - def submit_spam return unless akismet_enabled? && can_be_submitted? - spam!(user_agent_detail, spammable_text, creator) + spam!(user_agent_detail, spammable_text, owner) end - def spam?(env, user) - is_spam?(env, user, spammable_text) + def spam_detected?(env) + @spam = is_spam?(env, owner, spammable_text) end - def spam_detected? + def spam? @spam end def check_for_spam - self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam_detected? + self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam? + end + + def owner_id + if self.respond_to?(:author_id) + self.author_id + elsif self.respond_to?(:creator_id) + self.creator_id + end + end + + # Override this method if an additional check is needed before calling Akismet + def check_for_spam? + akismet_enabled? + end + + def spam_title + raise 'Implement in included model!' + end + + def spam_description + raise 'Implement in included model!' end private @@ -60,11 +76,7 @@ module Spammable result.reject(&:blank?).join("\n") end - def creator - if self.author_id - User.find(self.author_id) - elsif self.creator_id - User.find(self.creator_id) - end + def owner + User.find(owner_id) end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 5408e24b21c..40028e56489 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -265,4 +265,17 @@ class Issue < ActiveRecord::Base def overdue? due_date.try(:past?) || false end + + # To allow polymorphism with Spammable + def check_for_spam? + super && project.public? + end + + def spam_title + title + end + + def spam_description + description + end end diff --git a/app/services/create_spam_log_service.rb b/app/services/create_spam_log_service.rb deleted file mode 100644 index 59a66fde47a..00000000000 --- a/app/services/create_spam_log_service.rb +++ /dev/null @@ -1,13 +0,0 @@ -class CreateSpamLogService < BaseService - def initialize(project, user, params) - super(project, user, params) - end - - def execute - spam_params = params.merge({ user_id: @current_user.id, - project_id: @project.id } ) - spam_log = SpamLog.new(spam_params) - spam_log.save - spam_log - end -end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index d580834be83..9f8a642a75b 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -3,34 +3,34 @@ module Issues def execute filter_params label_params = params.delete(:label_ids) - request = params.delete(:request) - api = params.delete(:api) - issue = project.issues.new(params) - issue.author = params[:author] || current_user + @request = params.delete(:request) + @api = params.delete(:api) + @issue = project.issues.new(params) + @issue.author = params[:author] || current_user - issue.spam = spam_check_service.execute(request, api, issue) + spam_check_service.execute - if issue.save - issue.update_attributes(label_ids: label_params) - notification_service.new_issue(issue, current_user) - todo_service.new_issue(issue, current_user) - event_service.open_issue(issue, current_user) - user_agent_detail_service(issue, request).create - issue.create_cross_references!(current_user) - execute_hooks(issue, 'open') + if @issue.save + @issue.update_attributes(label_ids: label_params) + notification_service.new_issue(@issue, current_user) + todo_service.new_issue(@issue, current_user) + event_service.open_issue(@issue, current_user) + user_agent_detail_service.create + @issue.create_cross_references!(current_user) + execute_hooks(@issue, 'open') end - issue + @issue end private def spam_check_service - SpamCheckService.new(project, current_user, params) + SpamCheckService.new(@request, @api, @issue) end - def user_agent_detail_service(issue, request) - UserAgentDetailService.new(issue, request) + def user_agent_detail_service + UserAgentDetailService.new(@issue, @request) end end end diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb index 7d6754546a8..71b9436a22e 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -1,32 +1,33 @@ -class SpamCheckService < BaseService - attr_accessor :request, :api, :subject +class SpamCheckService + attr_accessor :request, :api, :spammable - def execute(request, api, subject) - @request, @api, @subject = request, api, subject - return false unless request || subject.check_for_spam?(project) - return false unless subject.spam?(request.env, current_user) - - create_spam_log + def initialize(request, api, spammable) + @request, @api, @spammable = request, api, spammable + end - true + def execute + if request && spammable.check_for_spam? + if spammable.spam_detected?(request.env) + create_spam_log + end + end end private def spam_log_attrs { - user_id: current_user.id, - project_id: project.id, - title: params[:title], - description: params[:description], - source_ip: subject.client_ip(request.env), - user_agent: subject.user_agent(request.env), - noteable_type: subject.class.to_s, + user_id: spammable.owner_id, + title: spammable.spam_title, + description: spammable.spam_description, + source_ip: spammable.client_ip(request.env), + user_agent: spammable.user_agent(request.env), + noteable_type: spammable.class.to_s, via_api: api } end def create_spam_log - CreateSpamLogService.new(project, current_user, spam_log_attrs).execute + SpamLog.create(spam_log_attrs) end end diff --git a/app/services/user_agent_detail_service.rb b/app/services/user_agent_detail_service.rb index dd995955be3..c07e2ca12a6 100644 --- a/app/services/user_agent_detail_service.rb +++ b/app/services/user_agent_detail_service.rb @@ -1,12 +1,12 @@ class UserAgentDetailService - attr_accessor :subject, :request + attr_accessor :spammable, :request - def initialize(subject, request) - @subject, @request = subject, request + def initialize(spammable, request) + @spammable, @request = spammable, request end def create return unless request - subject.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s) + spammable.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s) end end diff --git a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb new file mode 100644 index 00000000000..5950874d5af --- /dev/null +++ b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb @@ -0,0 +1,29 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveProjectIdFromSpamLogs < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = true + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + DOWNTIME_REASON = 'Removing a table that contains data that is not used anywhere.' + + # When using the methods "add_concurrent_index" or "add_column_with_default" + # you must disable the use of transactions as these methods can not run in an + # existing transaction. When using "add_concurrent_index" make sure that this + # method is the _only_ method called in the migration, any other changes + # should go in a separate migration. This ensures that upon failure _only_ the + # index creation fails and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + + def change + remove_column :spam_logs, :project_id, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 2e5ffac5935..cc881e54763 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -926,7 +926,6 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.string "source_ip" t.string "user_agent" t.boolean "via_api" - t.integer "project_id" t.string "noteable_type" t.string "title" t.text "description" diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 7bbfc137c2c..c4d3134da6c 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -160,7 +160,7 @@ module API issue = ::Issues::CreateService.new(project, current_user, attrs.merge(request: request, api: true)).execute - if issue.spam_detected? + if issue.spam? render_api_error!({ error: 'Spam detected' }, 400) end diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb index 19e73820321..b74d8176cc7 100644 --- a/lib/gitlab/akismet_helper.rb +++ b/lib/gitlab/akismet_helper.rb @@ -17,10 +17,6 @@ module Gitlab env['HTTP_USER_AGENT'] end - def check_for_spam?(project) - akismet_enabled? && project.public? - end - def is_spam?(environment, user, text) client = akismet_client ip_address = client_ip(environment) @@ -44,7 +40,7 @@ module Gitlab end end - def ham!(details, text, user) + def ham!(ip_address, user_agent, text, user) client = akismet_client params = { @@ -55,7 +51,7 @@ module Gitlab } begin - client.submit_ham(details.ip_address, details.user_agent, params) + client.submit_ham(ip_address, user_agent, params) rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 4e39826d694..efca838613f 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -274,7 +274,7 @@ describe Projects::IssuesController do describe 'POST #create' do context 'Akismet is enabled' do before do - allow_any_instance_of(Gitlab::AkismetHelper).to receive(:check_for_spam?).and_return(true) + allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true) allow_any_instance_of(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true) end diff --git a/spec/lib/gitlab/akismet_helper_spec.rb b/spec/lib/gitlab/akismet_helper_spec.rb index b08396da4d2..80b4f912d41 100644 --- a/spec/lib/gitlab/akismet_helper_spec.rb +++ b/spec/lib/gitlab/akismet_helper_spec.rb @@ -10,17 +10,6 @@ describe Gitlab::AkismetHelper, type: :helper do allow_any_instance_of(ApplicationSetting).to receive(:akismet_api_key).and_return('12345') end - describe '#check_for_spam?' do - it 'returns true for public project' do - expect(helper.check_for_spam?(project)).to eq(true) - end - - it 'returns false for private project' do - project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE) - expect(helper.check_for_spam?(project)).to eq(false) - end - end - describe '#is_spam?' do it 'returns true for spam' do environment = { diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index e61a6dcb69d..3f7d2721d22 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -15,7 +15,7 @@ describe Issue, 'Spammable' do describe 'InstanceMethods' do it 'should return the correct creator' do - expect(issue.send(:creator).id).to eq(issue.author_id) + expect(issue.send(:owner).id).to eq(issue.author_id) end it 'should be invalid if spam' do @@ -27,15 +27,28 @@ describe Issue, 'Spammable' do create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s) expect(issue.can_be_submitted?).to be_truthy end + + describe '#check_for_spam?' do + before do + allow_any_instance_of(Gitlab::AkismetHelper).to receive(:akismet_enabled?).and_return(true) + end + it 'returns true for public project' do + issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) + expect(issue.check_for_spam?).to eq(true) + end + + it 'returns false for other visibility levels' do + expect(issue.check_for_spam?).to eq(false) + end + end end describe 'AkismetMethods' do before do - allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(check_for_spam?: true, is_spam?: true, ham!: nil, spam!: nil) + allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(is_spam?: true, spam!: nil) end - it { expect(issue.spam?(:mock_env, :mock_user)).to be_truthy } + it { expect(issue.spam_detected?(:mock_env)).to be_truthy } it { expect(issue.submit_spam).to be_nil } - it { expect(issue.submit_ham).to be_nil } end end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 3cd4e981fb2..353b01d4a09 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -531,7 +531,7 @@ describe API::API, api: true do describe 'POST /projects/:id/issues with spam filtering' do before do - allow_any_instance_of(Gitlab::AkismetHelper).to receive(:check_for_spam?).and_return(true) + allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true) allow_any_instance_of(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true) end @@ -554,7 +554,6 @@ describe API::API, api: true do expect(spam_logs[0].description).to eq('content here') expect(spam_logs[0].user).to eq(user) expect(spam_logs[0].noteable_type).to eq('Issue') - expect(spam_logs[0].project_id).to eq(project.id) end end -- cgit v1.2.1 From abf2dcd25c4a176801314872733ede91297d1ab0 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 1 Aug 2016 12:14:03 -0500 Subject: Allow `SpamLog` to be submitted as ham - Added `submitted_as_ham` to `SpamLog` to mark which logs have been submitted to Akismet. - Added routes and controller action. --- app/controllers/admin/spam_logs_controller.rb | 11 +++++++++-- app/models/spam_log.rb | 4 ++++ app/views/admin/spam_logs/_spam_log.html.haml | 5 +++++ config/routes.rb | 6 +++++- ...160801163709_add_submitted_as_ham_to_spam_logs.rb | 20 ++++++++++++++++++++ db/schema.rb | 1 + lib/gitlab/akismet_helper.rb | 4 ++++ spec/models/concerns/spammable_spec.rb | 5 +++-- 8 files changed, 51 insertions(+), 5 deletions(-) create mode 100644 db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb index bc6fce0ec4e..d15f00bf84c 100644 --- a/app/controllers/admin/spam_logs_controller.rb +++ b/app/controllers/admin/spam_logs_controller.rb @@ -1,4 +1,6 @@ class Admin::SpamLogsController < Admin::ApplicationController + include Gitlab::AkismetHelper + def index @spam_logs = SpamLog.order(id: :desc).page(params[:page]) end @@ -15,9 +17,14 @@ class Admin::SpamLogsController < Admin::ApplicationController end end - def ham + def mark_as_ham spam_log = SpamLog.find(params[:id]) - Gitlab::AkismetHelper.ham!(spam_log.source_ip, spam_log.user_agent, spam_log.description, spam_log.user) + if ham!(spam_log.source_ip, spam_log.user_agent, spam_log.text, spam_log.user) + spam_log.update_attribute(:submitted_as_ham, true) + redirect_to admin_spam_logs_path, notice: 'Spam log successfully submitted as ham.' + else + redirect_to admin_spam_logs_path, notice: 'Error with Akismet. Please check the logs for more info.' + end end end diff --git a/app/models/spam_log.rb b/app/models/spam_log.rb index 12df68ef83b..3b8b9833565 100644 --- a/app/models/spam_log.rb +++ b/app/models/spam_log.rb @@ -7,4 +7,8 @@ class SpamLog < ActiveRecord::Base user.block user.destroy end + + def text + [title, description].join("\n") + end end diff --git a/app/views/admin/spam_logs/_spam_log.html.haml b/app/views/admin/spam_logs/_spam_log.html.haml index 8aea67f4497..f2b6106fb4a 100644 --- a/app/views/admin/spam_logs/_spam_log.html.haml +++ b/app/views/admin/spam_logs/_spam_log.html.haml @@ -24,6 +24,11 @@ = link_to 'Remove user', admin_spam_log_path(spam_log, remove_user: true), data: { confirm: "USER #{user.name} WILL BE REMOVED! Are you sure?" }, method: :delete, class: "btn btn-xs btn-remove" %td + - if spam_log.submitted_as_ham? + .btn.btn-xs.disabled + Submitted as Ham + - else + = link_to 'Submit as ham', mark_as_ham_admin_spam_log_path(spam_log), method: :post, class: 'btn btn-xs btn-warning' - if user && !user.blocked? = link_to 'Block user', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: "btn btn-xs" - else diff --git a/config/routes.rb b/config/routes.rb index 9a98fab15a3..8214bc26d59 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -252,7 +252,11 @@ Rails.application.routes.draw do resource :impersonation, only: :destroy resources :abuse_reports, only: [:index, :destroy] - resources :spam_logs, only: [:index, :destroy] + resources :spam_logs, only: [:index, :destroy] do + member do + post :mark_as_ham + end + end resources :applications diff --git a/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb b/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb new file mode 100644 index 00000000000..296f1dfac7b --- /dev/null +++ b/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb @@ -0,0 +1,20 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddSubmittedAsHamToSpamLogs < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + # DOWNTIME_REASON = '' + + disable_ddl_transaction! + + def change + add_column_with_default :spam_logs, :submitted_as_ham, :boolean, default: false + end +end diff --git a/db/schema.rb b/db/schema.rb index cc881e54763..355eed13b68 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -931,6 +931,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.text "description" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.boolean "submitted_as_ham", default: false, null: false end create_table "subscriptions", force: :cascade do |t| diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb index b74d8176cc7..bd71a1aaa51 100644 --- a/lib/gitlab/akismet_helper.rb +++ b/lib/gitlab/akismet_helper.rb @@ -52,8 +52,10 @@ module Gitlab begin client.submit_ham(ip_address, user_agent, params) + true rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") + false end end @@ -69,8 +71,10 @@ module Gitlab begin client.submit_spam(details.ip_address, details.user_agent, params) + true rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") + false end end end diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index 3f7d2721d22..7492c42f71e 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -45,10 +45,11 @@ describe Issue, 'Spammable' do describe 'AkismetMethods' do before do - allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(is_spam?: true, spam!: nil) + allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(is_spam?: true, spam!: true, akismet_enabled?: true) + allow_any_instance_of(Spammable).to receive(:can_be_submitted?).and_return(true) end it { expect(issue.spam_detected?(:mock_env)).to be_truthy } - it { expect(issue.submit_spam).to be_nil } + it { expect(issue.submit_spam).to be_truthy } end end -- cgit v1.2.1 From 96399a81cbb2e8a0f666241eeaff7cc784c26983 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 2 Aug 2016 16:21:57 -0500 Subject: Allow `Issue` to be submitted as spam - Added controller actions as reusable concerns - Added controller tests --- app/assets/stylesheets/framework/buttons.scss | 4 +++ app/controllers/concerns/spammable_actions.rb | 32 ++++++++++++++++++++++ app/controllers/projects/issues_controller.rb | 2 ++ app/models/concerns/spammable.rb | 18 ++++++++++-- app/services/system_note_service.rb | 17 ++++++++++++ app/views/admin/spam_logs/_spam_log.html.haml | 2 +- app/views/projects/issues/show.html.haml | 11 ++++++-- config/routes.rb | 1 + .../20160727163552_create_user_agent_details.rb | 1 + db/schema.rb | 1 + .../controllers/admin/spam_logs_controller_spec.rb | 12 ++++++++ .../controllers/projects/issues_controller_spec.rb | 29 ++++++++++++++++++++ spec/models/concerns/spammable_spec.rb | 9 +++--- 13 files changed, 129 insertions(+), 10 deletions(-) create mode 100644 app/controllers/concerns/spammable_actions.rb diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 473530cf094..f1fe1697d30 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -164,6 +164,10 @@ @include btn-outline($white-light, $orange-normal, $orange-normal, $orange-light, $white-light, $orange-light); } + &.btn-spam { + @include btn-outline($white-light, $red-normal, $red-normal, $red-light, $white-light, $red-light); + } + &.btn-danger, &.btn-remove, &.btn-red { diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb new file mode 100644 index 00000000000..85be25d84cc --- /dev/null +++ b/app/controllers/concerns/spammable_actions.rb @@ -0,0 +1,32 @@ +module SpammableActions + extend ActiveSupport::Concern + + included do + before_action :authorize_submit_spammable!, only: :mark_as_spam + end + + def mark_as_spam + if spammable.submit_spam + spammable.user_agent_detail.update_attribute(:submitted, true) + + if spammable.is_a?(Issuable) + SystemNoteService.submit_spam(spammable, spammable.project, current_user) + end + + redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.' + else + flash[:error] = 'Error with Akismet. Please check the logs for more info.' + redirect_to spammable + end + end + + private + + def spammable + raise NotImplementedError + end + + def authorize_submit_spammable! + access_denied! unless current_user.admin? + end +end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 660e0eba06f..e9fb11e8f94 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -4,6 +4,7 @@ class Projects::IssuesController < Projects::ApplicationController include IssuableActions include ToggleAwardEmoji include IssuableCollections + include SpammableActions before_action :redirect_to_external_issue_tracker, only: [:index, :new] before_action :module_enabled @@ -185,6 +186,7 @@ class Projects::IssuesController < Projects::ApplicationController alias_method :subscribable_resource, :issue alias_method :issuable, :issue alias_method :awardable, :issue + alias_method :spammable, :issue def authorize_read_issue! return render_404 unless can?(current_user, :read_issue, @issue) diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 5c75275b6e2..f272e7c5a55 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -22,7 +22,7 @@ module Spammable def can_be_submitted? if user_agent_detail - user_agent_detail.submittable? + user_agent_detail.submittable? && akismet_enabled? else false end @@ -41,6 +41,14 @@ module Spammable @spam end + def submitted? + if user_agent_detail + user_agent_detail.submitted + else + false + end + end + def check_for_spam self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam? end @@ -53,17 +61,21 @@ module Spammable end end + def to_ability_name + self.class.to_s.underscore + end + # Override this method if an additional check is needed before calling Akismet def check_for_spam? akismet_enabled? end def spam_title - raise 'Implement in included model!' + raise NotImplementedError end def spam_description - raise 'Implement in included model!' + raise NotImplementedError end private diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index e13dc9265b8..56d3329f5bd 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -395,6 +395,23 @@ module SystemNoteService create_note(noteable: noteable, project: project, author: author, note: body) end + # Called when the status of a Issuable is submitted as spam + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # + # Example Note text: + # + # "Issue submitted as spam." + # + # Returns the created Note object + def submit_spam(noteable, project, author) + body = "Submitted #{noteable.class.to_s.downcase} as spam" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + private def notes_for_mentioner(mentioner, noteable, notes) diff --git a/app/views/admin/spam_logs/_spam_log.html.haml b/app/views/admin/spam_logs/_spam_log.html.haml index f2b6106fb4a..4ce4eab8753 100644 --- a/app/views/admin/spam_logs/_spam_log.html.haml +++ b/app/views/admin/spam_logs/_spam_log.html.haml @@ -26,7 +26,7 @@ %td - if spam_log.submitted_as_ham? .btn.btn-xs.disabled - Submitted as Ham + Submitted as ham - else = link_to 'Submit as ham', mark_as_ham_admin_spam_log_path(spam_log), method: :post, class: 'btn btn-xs btn-warning' - if user && !user.blocked? diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index e5cce16a171..30e6a35db53 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -37,14 +37,21 @@ = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue' %li = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue) + - if @issue.can_be_submitted? && current_user.admin? + - unless @issue.submitted? + %li + = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'btn-spam', title: 'Submit as spam' + - if can?(current_user, :create_issue, @project) = link_to new_namespace_project_issue_path(@project.namespace, @project), class: 'hidden-xs hidden-sm btn btn-grouped new-issue-link btn-new btn-inverted', title: 'New issue', id: 'new_issue_link' do New issue - if can?(current_user, :update_issue, @issue) = link_to 'Reopen issue', issue_path(@issue, issue: { state_event: :reopen }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-reopen #{issue_button_visibility(@issue, false)}", title: 'Reopen issue' = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue' - = link_to edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit' do - Edit + = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit' + - if @issue.can_be_submitted? && current_user.admin? + - unless @issue.submitted? + = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'hidden-xs hidden-sm btn btn-grouped btn-spam', title: 'Submit as spam' .issue-details.issuable-details diff --git a/config/routes.rb b/config/routes.rb index 8214bc26d59..2cf2d111920 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -817,6 +817,7 @@ Rails.application.routes.draw do member do post :toggle_subscription post :toggle_award_emoji + post :mark_as_spam get :referenced_merge_requests get :related_branches get :can_create_branch diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb index 05c21a476fa..f9a02f310da 100644 --- a/db/migrate/20160727163552_create_user_agent_details.rb +++ b/db/migrate/20160727163552_create_user_agent_details.rb @@ -5,6 +5,7 @@ class CreateUserAgentDetails < ActiveRecord::Migration t.string :ip_address, null: false t.integer :subject_id, null: false t.string :subject_type, null: false + t.boolean :submitted, default: false t.timestamps null: false end diff --git a/db/schema.rb b/db/schema.rb index 355eed13b68..5ac08099e90 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1004,6 +1004,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.string "ip_address", null: false t.integer "subject_id", null: false t.string "subject_type", null: false + t.boolean "submitted", default: false t.datetime "created_at", null: false t.datetime "updated_at", null: false end diff --git a/spec/controllers/admin/spam_logs_controller_spec.rb b/spec/controllers/admin/spam_logs_controller_spec.rb index 520a4f6f9c5..f94afd1139d 100644 --- a/spec/controllers/admin/spam_logs_controller_spec.rb +++ b/spec/controllers/admin/spam_logs_controller_spec.rb @@ -34,4 +34,16 @@ describe Admin::SpamLogsController do expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) end end + + describe '#mark_as_ham' do + before do + allow_any_instance_of(Gitlab::AkismetHelper).to receive(:ham!).and_return(true) + end + it 'submits the log as ham' do + post :mark_as_ham, id: first_spam.id + + expect(response).to have_http_status(302) + expect(SpamLog.find(first_spam.id).submitted_as_ham).to be_truthy + end + end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index efca838613f..8fcde9a38bc 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -322,6 +322,35 @@ describe Projects::IssuesController do end end + describe 'POST #mark_as_spam' do + context 'properly submits to Akismet' do + before do + allow_any_instance_of(Spammable).to receive_messages(can_be_submitted?: true, submit_spam: true) + end + + def post_spam + admin = create(:admin) + create(:user_agent_detail, subject: issue) + project.team << [admin, :master] + sign_in(admin) + post :mark_as_spam, { + namespace_id: project.namespace.path, + project_id: project.path, + id: issue.iid + } + end + + it 'creates a system note' do + expect{ post_spam }.to change(Note, :count) + end + + it 'updates issue' do + post_spam + expect(issue.submitted?).to be_truthy + end + end + end + describe "DELETE #destroy" do context "when the user is a developer" do before { sign_in(user) } diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index 7492c42f71e..4e52d05918f 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -14,6 +14,10 @@ describe Issue, 'Spammable' do end describe 'InstanceMethods' do + before do + allow_any_instance_of(Gitlab::AkismetHelper).to receive(:akismet_enabled?).and_return(true) + end + it 'should return the correct creator' do expect(issue.send(:owner).id).to eq(issue.author_id) end @@ -24,14 +28,11 @@ describe Issue, 'Spammable' do end it 'should be submittable' do - create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s) + create(:user_agent_detail, subject: issue) expect(issue.can_be_submitted?).to be_truthy end describe '#check_for_spam?' do - before do - allow_any_instance_of(Gitlab::AkismetHelper).to receive(:akismet_enabled?).and_return(true) - end it 'returns true for public project' do issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) expect(issue.check_for_spam?).to eq(true) -- cgit v1.2.1 From 7179165af7553720089a0b7e7024374c371e2f90 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Thu, 4 Aug 2016 12:29:43 -0500 Subject: Added Documentation and CHANGELOG --- CHANGELOG | 1 + doc/integration/akismet.md | 23 +++++++++++++++++++++++ doc/integration/img/spam_log.png | Bin 0 -> 187190 bytes doc/integration/img/submit_issue.png | Bin 0 -> 176450 bytes 4 files changed, 24 insertions(+) create mode 100644 doc/integration/img/spam_log.png create mode 100644 doc/integration/img/submit_issue.png diff --git a/CHANGELOG b/CHANGELOG index ccc60846787..4aad4545509 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -81,6 +81,7 @@ v 8.11.0 (unreleased) - Make "New issue" button in Issue page less obtrusive !5457 (winniehell) - Gitlab::Metrics.current_transaction needs to be public for RailsQueueDuration - Fix search for notes which belongs to deleted objects + - Allow Akismet to be trained by submitting issues as spam or ham !5538 - Add GitLab Workhorse version to admin dashboard (Katarzyna Kobierska Ula Budziszewska) - Allow branch names ending with .json for graph and network page !5579 (winniehell) - Add the `sprockets-es6` gem diff --git a/doc/integration/akismet.md b/doc/integration/akismet.md index c222d21612f..06c787cfcc7 100644 --- a/doc/integration/akismet.md +++ b/doc/integration/akismet.md @@ -33,3 +33,26 @@ To use Akismet: 7. Save the configuration. ![Screenshot of Akismet settings](img/akismet_settings.png) + + +## Training + +> *Note:* Training the Akismet filter is only available in 8.11 and above. + +As a way to better recognize between spam and ham, you can train the Akismet +filter whenever there is a false positive or false negative. + +When an entry is recognized as spam, it is rejected and added to the Spam Logs. +From here you can review if they are really spam. If one of them is not really +spam, you can use the `Submit as ham` button to tell Akismet that it falsely +recognized an entry as spam. + +![Screenshot of Spam Logs](img/spam_log.png) + +If an entry that is actually spam was not recognized as such, you will be able +to also submit this to Akismet. The `Submit as spam` button will only appear +to admin users. + +![Screenshot of Issue](img/submit_issue.png) + +Training Akismet will help it to recognize spam more accurately in the future. diff --git a/doc/integration/img/spam_log.png b/doc/integration/img/spam_log.png new file mode 100644 index 00000000000..8d574448690 Binary files /dev/null and b/doc/integration/img/spam_log.png differ diff --git a/doc/integration/img/submit_issue.png b/doc/integration/img/submit_issue.png new file mode 100644 index 00000000000..9fd9c20f6b3 Binary files /dev/null and b/doc/integration/img/submit_issue.png differ -- cgit v1.2.1 From 43e756d4eafd79f4d2f366b646ebb94af78b5a4c Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Fri, 5 Aug 2016 17:10:08 -0500 Subject: Refactored AkismetHelper into AkismetService and cleaned up `Spammable` - Refactored SpamCheckService into SpamService --- app/controllers/admin/spam_logs_controller.rb | 4 +- app/controllers/concerns/spammable_actions.rb | 8 +-- app/models/concerns/spammable.rb | 68 ++++++++---------- app/models/issue.rb | 13 +--- app/services/akismet_service.rb | 78 +++++++++++++++++++++ app/services/issues/create_service.rb | 6 +- app/services/spam_check_service.rb | 33 --------- app/services/spam_service.rb | 64 +++++++++++++++++ app/services/system_note_service.rb | 4 +- .../20160727163552_create_user_agent_details.rb | 5 ++ ...60729173930_remove_project_id_from_spam_logs.rb | 2 +- lib/api/issues.rb | 2 - lib/gitlab/akismet_helper.rb | 81 ---------------------- .../controllers/admin/spam_logs_controller_spec.rb | 2 +- .../controllers/projects/issues_controller_spec.rb | 6 +- spec/lib/gitlab/akismet_helper_spec.rb | 24 ------- spec/models/concerns/spammable_spec.rb | 27 +++----- spec/requests/api/issues_spec.rb | 2 +- 18 files changed, 200 insertions(+), 229 deletions(-) create mode 100644 app/services/akismet_service.rb delete mode 100644 app/services/spam_check_service.rb create mode 100644 app/services/spam_service.rb delete mode 100644 lib/gitlab/akismet_helper.rb delete mode 100644 spec/lib/gitlab/akismet_helper_spec.rb diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb index d15f00bf84c..7876d2ee767 100644 --- a/app/controllers/admin/spam_logs_controller.rb +++ b/app/controllers/admin/spam_logs_controller.rb @@ -1,5 +1,4 @@ class Admin::SpamLogsController < Admin::ApplicationController - include Gitlab::AkismetHelper def index @spam_logs = SpamLog.order(id: :desc).page(params[:page]) @@ -20,8 +19,7 @@ class Admin::SpamLogsController < Admin::ApplicationController def mark_as_ham spam_log = SpamLog.find(params[:id]) - if ham!(spam_log.source_ip, spam_log.user_agent, spam_log.text, spam_log.user) - spam_log.update_attribute(:submitted_as_ham, true) + if SpamService.new(spam_log).mark_as_ham! redirect_to admin_spam_logs_path, notice: 'Spam log successfully submitted as ham.' else redirect_to admin_spam_logs_path, notice: 'Error with Akismet. Please check the logs for more info.' diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 85be25d84cc..296811267e5 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -6,13 +6,7 @@ module SpammableActions end def mark_as_spam - if spammable.submit_spam - spammable.user_agent_detail.update_attribute(:submitted, true) - - if spammable.is_a?(Issuable) - SystemNoteService.submit_spam(spammable, spammable.project, current_user) - end - + if SpamService.new(spammable).mark_as_spam!(current_user) redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.' else flash[:error] = 'Error with Akismet. Please check the logs for more info.' diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index f272e7c5a55..694e2efcade 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -1,55 +1,36 @@ module Spammable extend ActiveSupport::Concern - include Gitlab::AkismetHelper module ClassMethods - def attr_spammable(*attrs) - attrs.each do |attr| - spammable_attrs << attr.to_s - end + def attr_spammable(attr, options = {}) + spammable_attrs << [attr.to_s, options] end end included do has_one :user_agent_detail, as: :subject, dependent: :destroy attr_accessor :spam - after_validation :check_for_spam, on: :create + after_validation :spam_detected?, on: :create cattr_accessor :spammable_attrs, instance_accessor: false do [] end + delegate :submitted?, to: :user_agent_detail, allow_nil: true end def can_be_submitted? if user_agent_detail - user_agent_detail.submittable? && akismet_enabled? + user_agent_detail.submittable? else false end end - def submit_spam - return unless akismet_enabled? && can_be_submitted? - spam!(user_agent_detail, spammable_text, owner) - end - - def spam_detected?(env) - @spam = is_spam?(env, owner, spammable_text) - end - def spam? @spam end - def submitted? - if user_agent_detail - user_agent_detail.submitted - else - false - end - end - - def check_for_spam + def spam_detected? self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam? end @@ -61,34 +42,41 @@ module Spammable end end - def to_ability_name - self.class.to_s.underscore - end - - # Override this method if an additional check is needed before calling Akismet - def check_for_spam? - akismet_enabled? + def owner + User.find(owner_id) end def spam_title - raise NotImplementedError + attr = self.class.spammable_attrs.select do |_, options| + options.fetch(:spam_title, false) + end + + attr = attr[0].first + + public_send(attr) if respond_to?(attr.to_sym) end def spam_description - raise NotImplementedError - end + attr = self.class.spammable_attrs.select do |_, options| + options.fetch(:spam_description, false) + end - private + attr = attr[0].first + + public_send(attr) if respond_to?(attr.to_sym) + end def spammable_text result = [] - self.class.spammable_attrs.each do |entry| - result << self.send(entry) + self.class.spammable_attrs.map do |attr| + result << public_send(attr.first) end + result.reject(&:blank?).join("\n") end - def owner - User.find(owner_id) + # Override in Spammable if further checks are necessary + def check_for_spam? + current_application_settings.akismet_enabled end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 40028e56489..ab98d0cf9df 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -37,7 +37,8 @@ class Issue < ActiveRecord::Base scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') } scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') } - attr_spammable :title, :description + attr_spammable :title, spam_title: true + attr_spammable :description, spam_description: true state_machine :state, initial: :opened do event :close do @@ -266,16 +267,8 @@ class Issue < ActiveRecord::Base due_date.try(:past?) || false end - # To allow polymorphism with Spammable + # Only issues on public projects should be checked for spam def check_for_spam? super && project.public? end - - def spam_title - title - end - - def spam_description - description - end end diff --git a/app/services/akismet_service.rb b/app/services/akismet_service.rb new file mode 100644 index 00000000000..c09663bce85 --- /dev/null +++ b/app/services/akismet_service.rb @@ -0,0 +1,78 @@ +class AkismetService + attr_accessor :spammable + + def initialize(spammable) + @spammable = spammable + end + + def client_ip(env) + env['action_dispatch.remote_ip'].to_s + end + + def user_agent(env) + env['HTTP_USER_AGENT'] + end + + def is_spam?(environment) + ip_address = client_ip(environment) + user_agent = user_agent(environment) + + params = { + type: 'comment', + text: spammable.spammable_text, + created_at: DateTime.now, + author: spammable.owner.name, + author_email: spammable.owner.email, + referrer: environment['HTTP_REFERER'], + } + + begin + is_spam, is_blatant = akismet_client.check(ip_address, user_agent, params) + is_spam || is_blatant + rescue => e + Rails.logger.error("Unable to connect to Akismet: #{e}, skipping check") + false + end + end + + def ham! + params = { + type: 'comment', + text: spammable.text, + author: spammable.user.name, + author_email: spammable.user.email + } + + begin + akismet_client.submit_ham(spammable.source_ip, spammable.user_agent, params) + true + rescue => e + Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") + false + end + end + + def spam! + params = { + type: 'comment', + text: spammable.spammable_text, + author: spammable.owner.name, + author_email: spammable.owner.email + } + + begin + akismet_client.submit_spam(spammable.user_agent_detail.ip_address, spammable.user_agent_detail.user_agent, params) + true + rescue => e + Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") + false + end + end + + private + + def akismet_client + @akismet_client ||= ::Akismet::Client.new(current_application_settings.akismet_api_key, + Gitlab.config.gitlab.url) + end +end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 9f8a642a75b..67125d5c0e4 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -8,7 +8,7 @@ module Issues @issue = project.issues.new(params) @issue.author = params[:author] || current_user - spam_check_service.execute + @issue.spam = spam_service.check(@api, @request) if @issue.save @issue.update_attributes(label_ids: label_params) @@ -25,8 +25,8 @@ module Issues private - def spam_check_service - SpamCheckService.new(@request, @api, @issue) + def spam_service + SpamService.new(@issue) end def user_agent_detail_service diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb deleted file mode 100644 index 71b9436a22e..00000000000 --- a/app/services/spam_check_service.rb +++ /dev/null @@ -1,33 +0,0 @@ -class SpamCheckService - attr_accessor :request, :api, :spammable - - def initialize(request, api, spammable) - @request, @api, @spammable = request, api, spammable - end - - def execute - if request && spammable.check_for_spam? - if spammable.spam_detected?(request.env) - create_spam_log - end - end - end - - private - - def spam_log_attrs - { - user_id: spammable.owner_id, - title: spammable.spam_title, - description: spammable.spam_description, - source_ip: spammable.client_ip(request.env), - user_agent: spammable.user_agent(request.env), - noteable_type: spammable.class.to_s, - via_api: api - } - end - - def create_spam_log - SpamLog.create(spam_log_attrs) - end -end diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb new file mode 100644 index 00000000000..ad60de368aa --- /dev/null +++ b/app/services/spam_service.rb @@ -0,0 +1,64 @@ +class SpamService + attr_accessor :spammable + + def initialize(spammable) + @spammable = spammable + end + + def check(api, request) + return false unless request && spammable.check_for_spam? + return false unless akismet.is_spam?(request.env) + + create_spam_log(api, request) + true + end + + def mark_as_spam!(current_user) + return false unless akismet_enabled? && spammable.can_be_submitted? + if akismet.spam! + spammable.user_agent_detail.update_attribute(:submitted, true) + + if spammable.is_a?(Issuable) + SystemNoteService.submit_spam(spammable, spammable.project, current_user) + end + true + else + false + end + end + + def mark_as_ham! + return false unless spammable.is_a?(SpamLog) + + if akismet.ham! + spammable.update_attribute(:submitted_as_ham, true) + true + else + false + end + end + + private + + def akismet + @akismet ||= AkismetService.new(spammable) + end + + def akismet_enabled? + current_application_settings.akismet_enabled + end + + def create_spam_log(api, request) + SpamLog.create( + { + user_id: spammable.owner_id, + title: spammable.spam_title, + description: spammable.spam_description, + source_ip: akismet.client_ip(request.env), + user_agent: akismet.user_agent(request.env), + noteable_type: spammable.class.to_s, + via_api: api + } + ) + end +end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 56d3329f5bd..35c9ce909e6 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -395,7 +395,7 @@ module SystemNoteService create_note(noteable: noteable, project: project, author: author, note: body) end - # Called when the status of a Issuable is submitted as spam + # Called when Issuable is submitted as spam # # noteable - Noteable object # project - Project owning noteable @@ -407,7 +407,7 @@ module SystemNoteService # # Returns the created Note object def submit_spam(noteable, project, author) - body = "Submitted #{noteable.class.to_s.downcase} as spam" + body = "Submitted this #{noteable.class.to_s.downcase} as spam" create_note(noteable: noteable, project: project, author: author, note: body) end diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb index f9a02f310da..6677f5e80ba 100644 --- a/db/migrate/20160727163552_create_user_agent_details.rb +++ b/db/migrate/20160727163552_create_user_agent_details.rb @@ -1,4 +1,9 @@ class CreateUserAgentDetails < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + def change create_table :user_agent_details do |t| t.string :user_agent, null: false diff --git a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb index 5950874d5af..e28ab31d629 100644 --- a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb +++ b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb @@ -10,7 +10,7 @@ class RemoveProjectIdFromSpamLogs < ActiveRecord::Migration # When a migration requires downtime you **must** uncomment the following # constant and define a short and easy to understand explanation as to why the # migration requires downtime. - DOWNTIME_REASON = 'Removing a table that contains data that is not used anywhere.' + DOWNTIME_REASON = 'Removing a column that contains data that is not used anywhere.' # When using the methods "add_concurrent_index" or "add_column_with_default" # you must disable the use of transactions as these methods can not run in an diff --git a/lib/api/issues.rb b/lib/api/issues.rb index c4d3134da6c..077258faee1 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -3,8 +3,6 @@ module API class Issues < Grape::API before { authenticate! } - helpers ::Gitlab::AkismetHelper - helpers do def filter_issues_state(issues, state) case state diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb deleted file mode 100644 index bd71a1aaa51..00000000000 --- a/lib/gitlab/akismet_helper.rb +++ /dev/null @@ -1,81 +0,0 @@ -module Gitlab - module AkismetHelper - def akismet_enabled? - current_application_settings.akismet_enabled - end - - def akismet_client - @akismet_client ||= ::Akismet::Client.new(current_application_settings.akismet_api_key, - Gitlab.config.gitlab.url) - end - - def client_ip(env) - env['action_dispatch.remote_ip'].to_s - end - - def user_agent(env) - env['HTTP_USER_AGENT'] - end - - def is_spam?(environment, user, text) - client = akismet_client - ip_address = client_ip(environment) - user_agent = user_agent(environment) - - params = { - type: 'comment', - text: text, - created_at: DateTime.now, - author: user.name, - author_email: user.email, - referrer: environment['HTTP_REFERER'], - } - - begin - is_spam, is_blatant = client.check(ip_address, user_agent, params) - is_spam || is_blatant - rescue => e - Rails.logger.error("Unable to connect to Akismet: #{e}, skipping check") - false - end - end - - def ham!(ip_address, user_agent, text, user) - client = akismet_client - - params = { - type: 'comment', - text: text, - author: user.name, - author_email: user.email - } - - begin - client.submit_ham(ip_address, user_agent, params) - true - rescue => e - Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") - false - end - end - - def spam!(details, text, user) - client = akismet_client - - params = { - type: 'comment', - text: text, - author: user.name, - author_email: user.email - } - - begin - client.submit_spam(details.ip_address, details.user_agent, params) - true - rescue => e - Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") - false - end - end - end -end diff --git a/spec/controllers/admin/spam_logs_controller_spec.rb b/spec/controllers/admin/spam_logs_controller_spec.rb index f94afd1139d..ac0441d0a4e 100644 --- a/spec/controllers/admin/spam_logs_controller_spec.rb +++ b/spec/controllers/admin/spam_logs_controller_spec.rb @@ -37,7 +37,7 @@ describe Admin::SpamLogsController do describe '#mark_as_ham' do before do - allow_any_instance_of(Gitlab::AkismetHelper).to receive(:ham!).and_return(true) + allow_any_instance_of(AkismetService).to receive(:ham!).and_return(true) end it 'submits the log as ham' do post :mark_as_ham, id: first_spam.id diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 8fcde9a38bc..0e8d4b80b0e 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -275,7 +275,7 @@ describe Projects::IssuesController do context 'Akismet is enabled' do before do allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true) - allow_any_instance_of(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) end def post_spam_issue @@ -325,7 +325,9 @@ describe Projects::IssuesController do describe 'POST #mark_as_spam' do context 'properly submits to Akismet' do before do - allow_any_instance_of(Spammable).to receive_messages(can_be_submitted?: true, submit_spam: true) + allow_any_instance_of(AkismetService).to receive_messages(spam!: true) + allow_any_instance_of(ApplicationSetting).to receive_messages(akismet_enabled: true) + allow_any_instance_of(SpamService).to receive_messages(can_be_submitted?: true) end def post_spam diff --git a/spec/lib/gitlab/akismet_helper_spec.rb b/spec/lib/gitlab/akismet_helper_spec.rb deleted file mode 100644 index 80b4f912d41..00000000000 --- a/spec/lib/gitlab/akismet_helper_spec.rb +++ /dev/null @@ -1,24 +0,0 @@ -require 'spec_helper' - -describe Gitlab::AkismetHelper, type: :helper do - let(:project) { create(:project, :public) } - let(:user) { create(:user) } - - before do - allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url)) - allow_any_instance_of(ApplicationSetting).to receive(:akismet_enabled).and_return(true) - allow_any_instance_of(ApplicationSetting).to receive(:akismet_api_key).and_return('12345') - end - - describe '#is_spam?' do - it 'returns true for spam' do - environment = { - 'action_dispatch.remote_ip' => '127.0.0.1', - 'HTTP_USER_AGENT' => 'Test User Agent' - } - - allow_any_instance_of(::Akismet::Client).to receive(:check).and_return([true, true]) - expect(helper.is_spam?(environment, user, 'Is this spam?')).to eq(true) - end - end -end diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index 4e52d05918f..7944305e7b3 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -14,25 +14,24 @@ describe Issue, 'Spammable' do end describe 'InstanceMethods' do - before do - allow_any_instance_of(Gitlab::AkismetHelper).to receive(:akismet_enabled?).and_return(true) - end - it 'should return the correct creator' do - expect(issue.send(:owner).id).to eq(issue.author_id) + expect(issue.owner_id).to eq(issue.author_id) end it 'should be invalid if spam' do - issue.spam = true - expect(issue.valid?).to be_truthy + issue = build(:issue, spam: true) + expect(issue.valid?).to be_falsey end - it 'should be submittable' do + it 'should not be submitted' do create(:user_agent_detail, subject: issue) - expect(issue.can_be_submitted?).to be_truthy + expect(issue.submitted?).to be_falsey end describe '#check_for_spam?' do + before do + allow_any_instance_of(ApplicationSetting).to receive(:akismet_enabled).and_return(true) + end it 'returns true for public project' do issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) expect(issue.check_for_spam?).to eq(true) @@ -43,14 +42,4 @@ describe Issue, 'Spammable' do end end end - - describe 'AkismetMethods' do - before do - allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(is_spam?: true, spam!: true, akismet_enabled?: true) - allow_any_instance_of(Spammable).to receive(:can_be_submitted?).and_return(true) - end - - it { expect(issue.spam_detected?(:mock_env)).to be_truthy } - it { expect(issue.submit_spam).to be_truthy } - end end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 353b01d4a09..30b939c797c 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -532,7 +532,7 @@ describe API::API, api: true do describe 'POST /projects/:id/issues with spam filtering' do before do allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true) - allow_any_instance_of(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true) + allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true) end let(:params) do -- cgit v1.2.1 From 7638ebe4236cfdd0e7a76f87ba077acf57a7e806 Mon Sep 17 00:00:00 2001 From: Alfredo Sumaran Date: Fri, 12 Aug 2016 14:57:19 -0500 Subject: Restore `Largest repository` sort option on admin projects page --- app/helpers/sorting_helper.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/helpers/sorting_helper.rb b/app/helpers/sorting_helper.rb index e1c0b497550..8b138a8e69f 100644 --- a/app/helpers/sorting_helper.rb +++ b/app/helpers/sorting_helper.rb @@ -20,13 +20,19 @@ module SortingHelper end def projects_sort_options_hash - { + options = { sort_value_name => sort_title_name, sort_value_recently_updated => sort_title_recently_updated, sort_value_oldest_updated => sort_title_oldest_updated, sort_value_recently_created => sort_title_recently_created, sort_value_oldest_created => sort_title_oldest_created, } + + if current_controller?('admin/projects') + options.merge!(sort_value_largest_repo => sort_title_largest_repo) + end + + options end def sort_title_priority -- cgit v1.2.1 From cd8ba09ee8276b1f2c3ef186156c91ec92cddea3 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Mon, 15 Aug 2016 23:33:36 +0200 Subject: Make rubocop happy --- spec/models/project_services/irker_service_spec.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/models/project_services/irker_service_spec.rb b/spec/models/project_services/irker_service_spec.rb index a8f2d626477..ea718232255 100644 --- a/spec/models/project_services/irker_service_spec.rb +++ b/spec/models/project_services/irker_service_spec.rb @@ -55,15 +55,15 @@ describe IrkerService, models: true do @irker_server = TCPServer.new 'localhost', 0 allow(irker).to receive_messages( - active: true, - project: project, - project_id: project.id, - service_hook: true, - server_host: @irker_server.addr[2], - server_port: @irker_server.addr[1], - default_irc_uri: 'irc://chat.freenode.net/', - recipients: recipients, - colorize_messages: colorize_messages) + active: true, + project: project, + project_id: project.id, + service_hook: true, + server_host: @irker_server.addr[2], + server_port: @irker_server.addr[1], + default_irc_uri: 'irc://chat.freenode.net/', + recipients: recipients, + colorize_messages: colorize_messages) irker.valid? end -- cgit v1.2.1 From fb0a2e270f466f819a4c5cbe89067c48f88ca3f2 Mon Sep 17 00:00:00 2001 From: Connor Shea Date: Mon, 15 Aug 2016 12:20:48 -0600 Subject: Upgrade httpclient gem from 2.7.0.1 to 2.8.2. Fixes deprecation warnings from Ruby 2.3. Changelog: https://github.com/nahi/httpclient/blob/b51d7a8bb78f71726b08fbda5abfb900d627569f/CHANGELOG.md#changes-in-282 --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 3ba6048143c..2244c20203b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -338,7 +338,7 @@ GEM httparty (0.13.7) json (~> 1.8) multi_xml (>= 0.5.2) - httpclient (2.7.0.1) + httpclient (2.8.2) i18n (0.7.0) ice_nine (0.11.1) influxdb (0.2.3) -- cgit v1.2.1 From 5994c11910822463faeabb7b5f11d6529036db9d Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 9 Aug 2016 12:43:47 -0500 Subject: Further refactor and syntax fixes. --- app/controllers/admin/spam_logs_controller.rb | 5 +- app/controllers/concerns/spammable_actions.rb | 9 ++- app/models/concerns/spammable.rb | 42 ++++-------- app/models/issue.rb | 3 +- app/models/user_agent_detail.rb | 11 +--- app/services/akismet_service.rb | 59 ++++++++--------- app/services/ham_service.rb | 26 ++++++++ app/services/issues/create_service.rb | 4 +- app/services/spam_service.rb | 76 +++++++++++++--------- app/services/system_note_service.rb | 17 ----- app/services/user_agent_detail_service.rb | 1 + app/views/projects/issues/show.html.haml | 12 ++-- .../20160727163552_create_user_agent_details.rb | 2 +- db/schema.rb | 2 +- .../controllers/admin/spam_logs_controller_spec.rb | 2 +- .../controllers/projects/issues_controller_spec.rb | 13 ++-- spec/factories/user_agent_details.rb | 7 +- spec/models/concerns/spammable_spec.rb | 14 +--- spec/models/user_agent_detail_spec.rb | 22 +++++-- spec/requests/api/issues_spec.rb | 2 +- 20 files changed, 160 insertions(+), 169 deletions(-) create mode 100644 app/services/ham_service.rb diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb index 7876d2ee767..2abfa22712d 100644 --- a/app/controllers/admin/spam_logs_controller.rb +++ b/app/controllers/admin/spam_logs_controller.rb @@ -1,5 +1,4 @@ class Admin::SpamLogsController < Admin::ApplicationController - def index @spam_logs = SpamLog.order(id: :desc).page(params[:page]) end @@ -19,10 +18,10 @@ class Admin::SpamLogsController < Admin::ApplicationController def mark_as_ham spam_log = SpamLog.find(params[:id]) - if SpamService.new(spam_log).mark_as_ham! + if HamService.new(spam_log).mark_as_ham! redirect_to admin_spam_logs_path, notice: 'Spam log successfully submitted as ham.' else - redirect_to admin_spam_logs_path, notice: 'Error with Akismet. Please check the logs for more info.' + redirect_to admin_spam_logs_path, alert: 'Error with Akismet. Please check the logs for more info.' end end end diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 296811267e5..29e243c66a3 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -6,18 +6,17 @@ module SpammableActions end def mark_as_spam - if SpamService.new(spammable).mark_as_spam!(current_user) - redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.' + if SpamService.new(spammable).mark_as_spam! + redirect_to spammable, notice: "#{spammable.class.to_s} was submitted to Akismet successfully." else - flash[:error] = 'Error with Akismet. Please check the logs for more info.' - redirect_to spammable + redirect_to spammable, alert: 'Error with Akismet. Please check the logs for more info.' end end private def spammable - raise NotImplementedError + raise NotImplementedError, "#{self.class} does not implement #{__method__}" end def authorize_submit_spammable! diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 694e2efcade..ce54fe5d3bf 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -9,16 +9,19 @@ module Spammable included do has_one :user_agent_detail, as: :subject, dependent: :destroy + attr_accessor :spam - after_validation :spam_detected?, on: :create + + after_validation :check_for_spam, on: :create cattr_accessor :spammable_attrs, instance_accessor: false do [] end - delegate :submitted?, to: :user_agent_detail, allow_nil: true + + delegate :ip_address, :user_agent, to: :user_agent_detail, allow_nil: true end - def can_be_submitted? + def submittable_as_spam? if user_agent_detail user_agent_detail.submittable? else @@ -30,46 +33,29 @@ module Spammable @spam end - def spam_detected? + def check_for_spam self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam? end - def owner_id - if self.respond_to?(:author_id) - self.author_id - elsif self.respond_to?(:creator_id) - self.creator_id - end - end - - def owner - User.find(owner_id) - end - def spam_title - attr = self.class.spammable_attrs.select do |_, options| + attr = self.class.spammable_attrs.find do |_, options| options.fetch(:spam_title, false) end - attr = attr[0].first - - public_send(attr) if respond_to?(attr.to_sym) + public_send(attr.first) if attr && respond_to?(attr.first.to_sym) end def spam_description - attr = self.class.spammable_attrs.select do |_, options| + attr = self.class.spammable_attrs.find do |_, options| options.fetch(:spam_description, false) end - attr = attr[0].first - - public_send(attr) if respond_to?(attr.to_sym) + public_send(attr.first) if attr && respond_to?(attr.first.to_sym) end def spammable_text - result = [] - self.class.spammable_attrs.map do |attr| - result << public_send(attr.first) + result = self.class.spammable_attrs.map do |attr| + public_send(attr.first) end result.reject(&:blank?).join("\n") @@ -77,6 +63,6 @@ module Spammable # Override in Spammable if further checks are necessary def check_for_spam? - current_application_settings.akismet_enabled + true end end diff --git a/app/models/issue.rb b/app/models/issue.rb index ab98d0cf9df..788611305fe 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -8,7 +8,6 @@ class Issue < ActiveRecord::Base include Taskable include Spammable include FasterCacheKeys - include AkismetSubmittable DueDateStruct = Struct.new(:title, :name).freeze NoDueDate = DueDateStruct.new('No Due Date', '0').freeze @@ -269,6 +268,6 @@ class Issue < ActiveRecord::Base # Only issues on public projects should be checked for spam def check_for_spam? - super && project.public? + project.public? end end diff --git a/app/models/user_agent_detail.rb b/app/models/user_agent_detail.rb index 6d76dff20e3..0949c6ef083 100644 --- a/app/models/user_agent_detail.rb +++ b/app/models/user_agent_detail.rb @@ -1,16 +1,9 @@ class UserAgentDetail < ActiveRecord::Base belongs_to :subject, polymorphic: true - validates :user_agent, - presence: true - validates :ip_address, - presence: true - validates :subject_id, - presence: true - validates :subject_type, - presence: true + validates :user_agent, :ip_address, :subject_id, :subject_type, presence: true def submittable? - user_agent.present? && ip_address.present? + !submitted? end end diff --git a/app/services/akismet_service.rb b/app/services/akismet_service.rb index c09663bce85..5c60addbe7c 100644 --- a/app/services/akismet_service.rb +++ b/app/services/akismet_service.rb @@ -1,33 +1,26 @@ class AkismetService - attr_accessor :spammable + attr_accessor :owner, :text, :options - def initialize(spammable) - @spammable = spammable + def initialize(owner, text, options = {}) + @owner = owner + @text = text + @options = options end - def client_ip(env) - env['action_dispatch.remote_ip'].to_s - end - - def user_agent(env) - env['HTTP_USER_AGENT'] - end - - def is_spam?(environment) - ip_address = client_ip(environment) - user_agent = user_agent(environment) + def is_spam? + return false unless akismet_enabled? params = { type: 'comment', - text: spammable.spammable_text, + text: text, created_at: DateTime.now, - author: spammable.owner.name, - author_email: spammable.owner.email, - referrer: environment['HTTP_REFERER'], + author: owner.name, + author_email: owner.email, + referrer: options[:referrer], } begin - is_spam, is_blatant = akismet_client.check(ip_address, user_agent, params) + is_spam, is_blatant = akismet_client.check(options[:ip_address], options[:user_agent], params) is_spam || is_blatant rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping check") @@ -35,16 +28,18 @@ class AkismetService end end - def ham! + def submit_ham + return false unless akismet_enabled? + params = { type: 'comment', - text: spammable.text, - author: spammable.user.name, - author_email: spammable.user.email + text: text, + author: owner.name, + author_email: owner.email } begin - akismet_client.submit_ham(spammable.source_ip, spammable.user_agent, params) + akismet_client.submit_ham(options[:ip_address], options[:user_agent], params) true rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") @@ -52,16 +47,18 @@ class AkismetService end end - def spam! + def submit_spam + return false unless akismet_enabled? + params = { type: 'comment', - text: spammable.spammable_text, - author: spammable.owner.name, - author_email: spammable.owner.email + text: text, + author: owner.name, + author_email: owner.email } begin - akismet_client.submit_spam(spammable.user_agent_detail.ip_address, spammable.user_agent_detail.user_agent, params) + akismet_client.submit_spam(options[:ip_address], options[:user_agent], params) true rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") @@ -75,4 +72,8 @@ class AkismetService @akismet_client ||= ::Akismet::Client.new(current_application_settings.akismet_api_key, Gitlab.config.gitlab.url) end + + def akismet_enabled? + current_application_settings.akismet_enabled + end end diff --git a/app/services/ham_service.rb b/app/services/ham_service.rb new file mode 100644 index 00000000000..b0e1799b489 --- /dev/null +++ b/app/services/ham_service.rb @@ -0,0 +1,26 @@ +class HamService + attr_accessor :spam_log + + def initialize(spam_log) + @spam_log = spam_log + end + + def mark_as_ham! + if akismet.submit_ham + spam_log.update_attribute(:submitted_as_ham, true) + else + false + end + end + + private + + def akismet + @akismet ||= AkismetService.new( + spam_log.user, + spam_log.text, + ip_address: spam_log.source_ip, + user_agent: spam_log.user_agent + ) + end +end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 67125d5c0e4..65550ab8ec6 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -8,7 +8,7 @@ module Issues @issue = project.issues.new(params) @issue.author = params[:author] || current_user - @issue.spam = spam_service.check(@api, @request) + @issue.spam = spam_service.check(@api) if @issue.save @issue.update_attributes(label_ids: label_params) @@ -26,7 +26,7 @@ module Issues private def spam_service - SpamService.new(@issue) + SpamService.new(@issue, @request) end def user_agent_detail_service diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb index ad60de368aa..48903291799 100644 --- a/app/services/spam_service.rb +++ b/app/services/spam_service.rb @@ -1,61 +1,75 @@ class SpamService - attr_accessor :spammable + attr_accessor :spammable, :request, :options - def initialize(spammable) + def initialize(spammable, request = nil) @spammable = spammable + @request = request + @options = {} + + if @request + @options[:ip_address] = @request.env['action_dispatch.remote_ip'].to_s + @options[:user_agent] = @request.env['HTTP_USER_AGENT'] + @options[:referrer] = @request.env['HTTP_REFERRER'] + else + @options[:ip_address] = @spammable.ip_address + @options[:user_agent] = @spammable.user_agent + end end - def check(api, request) - return false unless request && spammable.check_for_spam? - return false unless akismet.is_spam?(request.env) + def check(api = false) + return false unless request && check_for_spam? - create_spam_log(api, request) + return false unless akismet.is_spam? + + create_spam_log(api) true end - def mark_as_spam!(current_user) - return false unless akismet_enabled? && spammable.can_be_submitted? - if akismet.spam! - spammable.user_agent_detail.update_attribute(:submitted, true) + def mark_as_spam! + return false unless spammable.submittable_as_spam? - if spammable.is_a?(Issuable) - SystemNoteService.submit_spam(spammable, spammable.project, current_user) - end - true + if akismet.submit_spam + spammable.user_agent_detail.update_attribute(:submitted, true) else false end end - def mark_as_ham! - return false unless spammable.is_a?(SpamLog) + private - if akismet.ham! - spammable.update_attribute(:submitted_as_ham, true) - true - else - false - end + def akismet + @akismet ||= AkismetService.new( + spammable_owner, + spammable.spammable_text, + options + ) end - private + def spammable_owner + @user ||= User.find(spammable_owner_id) + end - def akismet - @akismet ||= AkismetService.new(spammable) + def spammable_owner_id + @owner_id ||= + if spammable.respond_to?(:author_id) + spammable.author_id + elsif spammable.respond_to?(:creator_id) + spammable.creator_id + end end - def akismet_enabled? - current_application_settings.akismet_enabled + def check_for_spam? + spammable.check_for_spam? end - def create_spam_log(api, request) + def create_spam_log(api) SpamLog.create( { - user_id: spammable.owner_id, + user_id: spammable_owner_id, title: spammable.spam_title, description: spammable.spam_description, - source_ip: akismet.client_ip(request.env), - user_agent: akismet.user_agent(request.env), + source_ip: options[:ip_address], + user_agent: options[:user_agent], noteable_type: spammable.class.to_s, via_api: api } diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 35c9ce909e6..e13dc9265b8 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -395,23 +395,6 @@ module SystemNoteService create_note(noteable: noteable, project: project, author: author, note: body) end - # Called when Issuable is submitted as spam - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # - # Example Note text: - # - # "Issue submitted as spam." - # - # Returns the created Note object - def submit_spam(noteable, project, author) - body = "Submitted this #{noteable.class.to_s.downcase} as spam" - - create_note(noteable: noteable, project: project, author: author, note: body) - end - private def notes_for_mentioner(mentioner, noteable, notes) diff --git a/app/services/user_agent_detail_service.rb b/app/services/user_agent_detail_service.rb index c07e2ca12a6..a1ee3df5fe1 100644 --- a/app/services/user_agent_detail_service.rb +++ b/app/services/user_agent_detail_service.rb @@ -7,6 +7,7 @@ class UserAgentDetailService def create return unless request + spammable.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s) end end diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index 30e6a35db53..9f1a046ea74 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -37,10 +37,9 @@ = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue' %li = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue) - - if @issue.can_be_submitted? && current_user.admin? - - unless @issue.submitted? - %li - = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'btn-spam', title: 'Submit as spam' + - if @issue.submittable_as_spam? && current_user.admin? + %li + = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'btn-spam', title: 'Submit as spam' - if can?(current_user, :create_issue, @project) = link_to new_namespace_project_issue_path(@project.namespace, @project), class: 'hidden-xs hidden-sm btn btn-grouped new-issue-link btn-new btn-inverted', title: 'New issue', id: 'new_issue_link' do @@ -48,10 +47,9 @@ - if can?(current_user, :update_issue, @issue) = link_to 'Reopen issue', issue_path(@issue, issue: { state_event: :reopen }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-reopen #{issue_button_visibility(@issue, false)}", title: 'Reopen issue' = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue' - = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit' - - if @issue.can_be_submitted? && current_user.admin? - - unless @issue.submitted? + - if @issue.submittable_as_spam? && current_user.admin? = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'hidden-xs hidden-sm btn btn-grouped btn-spam', title: 'Submit as spam' + = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit' .issue-details.issuable-details diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb index 6677f5e80ba..ed4ccfedc0a 100644 --- a/db/migrate/20160727163552_create_user_agent_details.rb +++ b/db/migrate/20160727163552_create_user_agent_details.rb @@ -10,7 +10,7 @@ class CreateUserAgentDetails < ActiveRecord::Migration t.string :ip_address, null: false t.integer :subject_id, null: false t.string :subject_type, null: false - t.boolean :submitted, default: false + t.boolean :submitted, default: false, null: false t.timestamps null: false end diff --git a/db/schema.rb b/db/schema.rb index 5ac08099e90..52ba60ace11 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1004,7 +1004,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.string "ip_address", null: false t.integer "subject_id", null: false t.string "subject_type", null: false - t.boolean "submitted", default: false + t.boolean "submitted", default: false, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false end diff --git a/spec/controllers/admin/spam_logs_controller_spec.rb b/spec/controllers/admin/spam_logs_controller_spec.rb index ac0441d0a4e..585ca31389d 100644 --- a/spec/controllers/admin/spam_logs_controller_spec.rb +++ b/spec/controllers/admin/spam_logs_controller_spec.rb @@ -37,7 +37,7 @@ describe Admin::SpamLogsController do describe '#mark_as_ham' do before do - allow_any_instance_of(AkismetService).to receive(:ham!).and_return(true) + allow_any_instance_of(AkismetService).to receive(:submit_ham).and_return(true) end it 'submits the log as ham' do post :mark_as_ham, id: first_spam.id diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 0e8d4b80b0e..0836b71056c 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -274,7 +274,7 @@ describe Projects::IssuesController do describe 'POST #create' do context 'Akismet is enabled' do before do - allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true) + allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) end @@ -317,7 +317,7 @@ describe Projects::IssuesController do end it 'creates a user agent detail' do - expect{ post_new_issue }.to change(UserAgentDetail, :count) + expect{ post_new_issue }.to change(UserAgentDetail, :count).by(1) end end end @@ -325,9 +325,8 @@ describe Projects::IssuesController do describe 'POST #mark_as_spam' do context 'properly submits to Akismet' do before do - allow_any_instance_of(AkismetService).to receive_messages(spam!: true) + allow_any_instance_of(AkismetService).to receive_messages(submit_spam: true) allow_any_instance_of(ApplicationSetting).to receive_messages(akismet_enabled: true) - allow_any_instance_of(SpamService).to receive_messages(can_be_submitted?: true) end def post_spam @@ -342,13 +341,9 @@ describe Projects::IssuesController do } end - it 'creates a system note' do - expect{ post_spam }.to change(Note, :count) - end - it 'updates issue' do post_spam - expect(issue.submitted?).to be_truthy + expect(issue.submittable_as_spam?).to be_falsey end end end diff --git a/spec/factories/user_agent_details.rb b/spec/factories/user_agent_details.rb index 10de5dcb329..9763cc0cf15 100644 --- a/spec/factories/user_agent_details.rb +++ b/spec/factories/user_agent_details.rb @@ -2,11 +2,6 @@ FactoryGirl.define do factory :user_agent_detail do ip_address '127.0.0.1' user_agent 'AppleWebKit/537.36' - subject_id 1 - subject_type 'Issue' - - trait :on_issue do - association :subject, factory: :issue - end + association :subject, factory: :issue end end diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index 7944305e7b3..32935bc0b09 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -9,29 +9,17 @@ describe Issue, 'Spammable' do describe 'ClassMethods' do it 'should return correct attr_spammable' do - expect(issue.send(:spammable_text)).to eq("#{issue.title}\n#{issue.description}") + expect(issue.spammable_text).to eq("#{issue.title}\n#{issue.description}") end end describe 'InstanceMethods' do - it 'should return the correct creator' do - expect(issue.owner_id).to eq(issue.author_id) - end - it 'should be invalid if spam' do issue = build(:issue, spam: true) expect(issue.valid?).to be_falsey end - it 'should not be submitted' do - create(:user_agent_detail, subject: issue) - expect(issue.submitted?).to be_falsey - end - describe '#check_for_spam?' do - before do - allow_any_instance_of(ApplicationSetting).to receive(:akismet_enabled).and_return(true) - end it 'returns true for public project' do issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) expect(issue.check_for_spam?).to eq(true) diff --git a/spec/models/user_agent_detail_spec.rb b/spec/models/user_agent_detail_spec.rb index ba21161fc7f..a8c25766e73 100644 --- a/spec/models/user_agent_detail_spec.rb +++ b/spec/models/user_agent_detail_spec.rb @@ -2,16 +2,30 @@ require 'rails_helper' describe UserAgentDetail, type: :model do describe '.submittable?' do - it 'should be submittable' do - detail = create(:user_agent_detail, :on_issue) + it 'is submittable when not already submitted' do + detail = build(:user_agent_detail) + expect(detail.submittable?).to be_truthy end + + it 'is not submittable when already submitted' do + detail = build(:user_agent_detail, submitted: true) + + expect(detail.submittable?).to be_falsey + end end describe '.valid?' do - it 'should be valid with a subject' do - detail = create(:user_agent_detail, :on_issue) + it 'is valid with a subject' do + detail = build(:user_agent_detail) + expect(detail).to be_valid end + + it 'is invalid without a subject' do + detail = build(:user_agent_detail, subject: nil) + + expect(detail).not_to be_valid + end end end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 30b939c797c..a40e1a93b71 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -531,7 +531,7 @@ describe API::API, api: true do describe 'POST /projects/:id/issues with spam filtering' do before do - allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true) + allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true) end -- cgit v1.2.1 From e805a6470031d942f7de604fdf7acfc7cf4f0b1a Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 10:39:13 +0530 Subject: Backport changes from gitlab-org/gitlab-ee!581 to CE. !581 has a lot of changes that would cause merge conflicts if not properly backported to CE. This commit/MR serves as a better foundation for gitlab-org/gitlab-ee!581. = Changes = 1. Move from `has_one {merge,push}_access_level` to `has_many`, with the `length` of the association limited to `1`. This is _effectively_ a `has_one` association, but should cause less conflicts with EE, which is set to `has_many`. This has a number of related changes in the views, specs, and factories. 2. Make `gon` variable loading more consistent (with EE!581) in the `ProtectedBranchesController`. Also use `::` to prefix the `ProtectedBranches` services, because this is required in EE. 3. Extract a `ProtectedBranchAccess` concern from the two access level models. This concern only has a single `humanize` method here, but will have more methods in EE. 4. Add `form_errors` to the protected branches creation form. This is not strictly required for EE compatibility, but was an oversight nonetheless. --- .../javascripts/protected_branch_create.js.es6 | 4 ++-- .../javascripts/protected_branch_edit.js.es6 | 10 +++++---- .../projects/protected_branches_controller.rb | 26 +++++++++++++--------- app/models/concerns/protected_branch_access.rb | 7 ++++++ app/models/protected_branch.rb | 11 +++++---- app/models/protected_branch/merge_access_level.rb | 6 ++--- app/models/protected_branch/push_access_level.rb | 6 ++--- app/services/protected_branches/create_service.rb | 18 +-------------- .../_create_protected_branch.html.haml | 9 ++++---- .../protected_branches/_protected_branch.html.haml | 12 +++++----- lib/api/entities.rb | 4 ++-- spec/factories/protected_branches.rb | 10 ++++----- spec/features/protected_branches_spec.rb | 13 ++++++----- spec/services/git_push_service_spec.rb | 6 ++--- 14 files changed, 72 insertions(+), 70 deletions(-) create mode 100644 app/models/concerns/protected_branch_access.rb diff --git a/app/assets/javascripts/protected_branch_create.js.es6 b/app/assets/javascripts/protected_branch_create.js.es6 index 00e20a03b04..2efca2414dc 100644 --- a/app/assets/javascripts/protected_branch_create.js.es6 +++ b/app/assets/javascripts/protected_branch_create.js.es6 @@ -44,8 +44,8 @@ // Enable submit button const $branchInput = this.$wrap.find('input[name="protected_branch[name]"]'); - const $allowedToMergeInput = this.$wrap.find('input[name="protected_branch[merge_access_level_attributes][access_level]"]'); - const $allowedToPushInput = this.$wrap.find('input[name="protected_branch[push_access_level_attributes][access_level]"]'); + const $allowedToMergeInput = this.$wrap.find('input[name="protected_branch[merge_access_levels_attributes][0][access_level]"]'); + const $allowedToPushInput = this.$wrap.find('input[name="protected_branch[push_access_levels_attributes][0][access_level]"]'); if ($branchInput.val() && $allowedToMergeInput.val() && $allowedToPushInput.val()){ this.$form.find('input[type="submit"]').removeAttr('disabled'); diff --git a/app/assets/javascripts/protected_branch_edit.js.es6 b/app/assets/javascripts/protected_branch_edit.js.es6 index 8d42e268ebc..a59fcbfa082 100644 --- a/app/assets/javascripts/protected_branch_edit.js.es6 +++ b/app/assets/javascripts/protected_branch_edit.js.es6 @@ -39,12 +39,14 @@ _method: 'PATCH', id: this.$wrap.data('banchId'), protected_branch: { - merge_access_level_attributes: { + merge_access_levels_attributes: [{ + id: this.$allowedToMergeDropdown.data('access-level-id'), access_level: $allowedToMergeInput.val() - }, - push_access_level_attributes: { + }], + push_access_levels_attributes: [{ + id: this.$allowedToPushDropdown.data('access-level-id'), access_level: $allowedToPushInput.val() - } + }] } }, success: () => { diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index d28ec6e2eac..9a438d5512c 100644 --- a/app/controllers/projects/protected_branches_controller.rb +++ b/app/controllers/projects/protected_branches_controller.rb @@ -9,16 +9,16 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController def index @protected_branch = @project.protected_branches.new - load_protected_branches_gon_variables + load_gon_index end def create - @protected_branch = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute + @protected_branch = ::ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute if @protected_branch.persisted? redirect_to namespace_project_protected_branches_path(@project.namespace, @project) else load_protected_branches - load_protected_branches_gon_variables + load_gon_index render :index end end @@ -28,7 +28,7 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController end def update - @protected_branch = ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch) + @protected_branch = ::ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch) if @protected_branch.valid? respond_to do |format| @@ -58,17 +58,23 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController def protected_branch_params params.require(:protected_branch).permit(:name, - merge_access_level_attributes: [:access_level], - push_access_level_attributes: [:access_level]) + merge_access_levels_attributes: [:access_level, :id], + push_access_levels_attributes: [:access_level, :id]) end def load_protected_branches @protected_branches = @project.protected_branches.order(:name).page(params[:page]) end - def load_protected_branches_gon_variables - gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } }, - push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } }, - merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } }) + def access_levels_options + { + push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text, before_divider: true } }, + merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text, before_divider: true } } + } + end + + def load_gon_index + params = { open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } } + gon.push(params.merge(access_levels_options)) end end diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb new file mode 100644 index 00000000000..5a7b36070e7 --- /dev/null +++ b/app/models/concerns/protected_branch_access.rb @@ -0,0 +1,7 @@ +module ProtectedBranchAccess + extend ActiveSupport::Concern + + def humanize + self.class.human_access_levels[self.access_level] + end +end diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 226b3f54342..6240912a6e1 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -5,11 +5,14 @@ class ProtectedBranch < ActiveRecord::Base validates :name, presence: true validates :project, presence: true - has_one :merge_access_level, dependent: :destroy - has_one :push_access_level, dependent: :destroy + has_many :merge_access_levels, dependent: :destroy + has_many :push_access_levels, dependent: :destroy - accepts_nested_attributes_for :push_access_level - accepts_nested_attributes_for :merge_access_level + validates_length_of :merge_access_levels, is: 1, message: "are restricted to a single instance per protected branch." + validates_length_of :push_access_levels, is: 1, message: "are restricted to a single instance per protected branch." + + accepts_nested_attributes_for :push_access_levels + accepts_nested_attributes_for :merge_access_levels def commit project.commit(self.name) diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb index b1112ee737d..806b3ccd275 100644 --- a/app/models/protected_branch/merge_access_level.rb +++ b/app/models/protected_branch/merge_access_level.rb @@ -1,4 +1,6 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base + include ProtectedBranchAccess + belongs_to :protected_branch delegate :project, to: :protected_branch @@ -17,8 +19,4 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base project.team.max_member_access(user.id) >= access_level end - - def humanize - self.class.human_access_levels[self.access_level] - end end diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb index 6a5e49cf453..92e9c51d883 100644 --- a/app/models/protected_branch/push_access_level.rb +++ b/app/models/protected_branch/push_access_level.rb @@ -1,4 +1,6 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base + include ProtectedBranchAccess + belongs_to :protected_branch delegate :project, to: :protected_branch @@ -20,8 +22,4 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base project.team.max_member_access(user.id) >= access_level end - - def humanize - self.class.human_access_levels[self.access_level] - end end diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb index 6150a2a83c9..a84e335340d 100644 --- a/app/services/protected_branches/create_service.rb +++ b/app/services/protected_branches/create_service.rb @@ -5,23 +5,7 @@ module ProtectedBranches def execute raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) - protected_branch = project.protected_branches.new(params) - - ProtectedBranch.transaction do - protected_branch.save! - - if protected_branch.push_access_level.blank? - protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER) - end - - if protected_branch.merge_access_level.blank? - protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER) - end - end - - protected_branch - rescue ActiveRecord::RecordInvalid - protected_branch + project.protected_branches.create(params) end end end diff --git a/app/views/projects/protected_branches/_create_protected_branch.html.haml b/app/views/projects/protected_branches/_create_protected_branch.html.haml index 85d0c494ba8..d4c6fa24768 100644 --- a/app/views/projects/protected_branches/_create_protected_branch.html.haml +++ b/app/views/projects/protected_branches/_create_protected_branch.html.haml @@ -5,6 +5,7 @@ Protect a branch .panel-body .form-horizontal + = form_errors(@protected_branch) .form-group = f.label :name, class: 'col-md-2 text-right' do Branch: @@ -18,19 +19,19 @@ %code production/* are supported .form-group - %label.col-md-2.text-right{ for: 'merge_access_level_attributes' } + %label.col-md-2.text-right{ for: 'merge_access_levels_attributes' } Allowed to merge: .col-md-10 = dropdown_tag('Select', options: { toggle_class: 'js-allowed-to-merge wide', - data: { field_name: 'protected_branch[merge_access_level_attributes][access_level]', input_id: 'merge_access_level_attributes' }}) + data: { field_name: 'protected_branch[merge_access_levels_attributes][0][access_level]', input_id: 'merge_access_levels_attributes' }}) .form-group - %label.col-md-2.text-right{ for: 'push_access_level_attributes' } + %label.col-md-2.text-right{ for: 'push_access_levels_attributes' } Allowed to push: .col-md-10 = dropdown_tag('Select', options: { toggle_class: 'js-allowed-to-push wide', - data: { field_name: 'protected_branch[push_access_level_attributes][access_level]', input_id: 'push_access_level_attributes' }}) + data: { field_name: 'protected_branch[push_access_levels_attributes][0][access_level]', input_id: 'push_access_levels_attributes' }}) .panel-footer = f.submit 'Protect', class: 'btn-create btn', disabled: true diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml index e2e01ee78f8..eb4c67daa80 100644 --- a/app/views/projects/protected_branches/_protected_branch.html.haml +++ b/app/views/projects/protected_branches/_protected_branch.html.haml @@ -14,15 +14,15 @@ - else (branch was removed from repository) %td - = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level - = dropdown_tag( (protected_branch.merge_access_level.humanize || 'Select') , + = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_levels.first.access_level + = dropdown_tag( (protected_branch.merge_access_levels.first.humanize || 'Select') , options: { toggle_class: 'js-allowed-to-merge', dropdown_class: 'dropdown-menu-selectable js-allowed-to-merge-container', - data: { field_name: "allowed_to_merge_#{protected_branch.id}" }}) + data: { field_name: "allowed_to_merge_#{protected_branch.id}", access_level_id: protected_branch.merge_access_levels.first.id }}) %td - = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level - = dropdown_tag( (protected_branch.push_access_level.humanize || 'Select') , + = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_levels.first.access_level + = dropdown_tag( (protected_branch.push_access_levels.first.humanize || 'Select') , options: { toggle_class: 'js-allowed-to-push', dropdown_class: 'dropdown-menu-selectable js-allowed-to-push-container', - data: { field_name: "allowed_to_push_#{protected_branch.id}" }}) + data: { field_name: "allowed_to_push_#{protected_branch.id}", access_level_id: protected_branch.push_access_levels.first.id }}) - if can_admin_project %td = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: 'btn btn-warning' diff --git a/lib/api/entities.rb b/lib/api/entities.rb index ae74d14a4bb..7bce427adf6 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -129,12 +129,12 @@ module API expose :developers_can_push do |repo_branch, options| project = options[:project] - project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_level.access_level == Gitlab::Access::DEVELOPER } + project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_levels.first.access_level == Gitlab::Access::DEVELOPER } end expose :developers_can_merge do |repo_branch, options| project = options[:project] - project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.access_level == Gitlab::Access::DEVELOPER } + project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_levels.first.access_level == Gitlab::Access::DEVELOPER } end end diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb index 5575852c2d7..3b21174987f 100644 --- a/spec/factories/protected_branches.rb +++ b/spec/factories/protected_branches.rb @@ -4,25 +4,25 @@ FactoryGirl.define do project after(:create) do |protected_branch| - protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER) - protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER) + protected_branch.push_access_levels.create!(access_level: Gitlab::Access::MASTER) + protected_branch.merge_access_levels.create!(access_level: Gitlab::Access::MASTER) end trait :developers_can_push do after(:create) do |protected_branch| - protected_branch.push_access_level.update!(access_level: Gitlab::Access::DEVELOPER) + protected_branch.push_access_levels.first.update!(access_level: Gitlab::Access::DEVELOPER) end end trait :developers_can_merge do after(:create) do |protected_branch| - protected_branch.merge_access_level.update!(access_level: Gitlab::Access::DEVELOPER) + protected_branch.merge_access_levels.first.update!(access_level: Gitlab::Access::DEVELOPER) end end trait :no_one_can_push do after(:create) do |protected_branch| - protected_branch.push_access_level.update!(access_level: Gitlab::Access::NO_ACCESS) + protected_branch.push_access_levels.first.update!(access_level: Gitlab::Access::NO_ACCESS) end end end diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb index 3499460c84d..5beb658b2f5 100644 --- a/spec/features/protected_branches_spec.rb +++ b/spec/features/protected_branches_spec.rb @@ -71,7 +71,10 @@ feature 'Projected Branches', feature: true, js: true do project.repository.add_branch(user, 'production-stable', 'master') project.repository.add_branch(user, 'staging-stable', 'master') project.repository.add_branch(user, 'development', 'master') - create(:protected_branch, project: project, name: "*-stable") + + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('*-stable') + click_on "Protect" visit namespace_project_protected_branches_path(project.namespace, project) click_on "2 matching branches" @@ -96,7 +99,7 @@ feature 'Projected Branches', feature: true, js: true do click_on "Protect" expect(ProtectedBranch.count).to eq(1) - expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id) + expect(ProtectedBranch.last.push_access_levels.first.access_level).to eq(access_type_id) end it "allows updating protected branches so that #{access_type_name} can push to them" do @@ -112,7 +115,7 @@ feature 'Projected Branches', feature: true, js: true do end wait_for_ajax - expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id) + expect(ProtectedBranch.last.push_access_levels.first.access_level).to eq(access_type_id) end end @@ -127,7 +130,7 @@ feature 'Projected Branches', feature: true, js: true do click_on "Protect" expect(ProtectedBranch.count).to eq(1) - expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id) + expect(ProtectedBranch.last.merge_access_levels.first.access_level).to eq(access_type_id) end it "allows updating protected branches so that #{access_type_name} can merge to them" do @@ -143,7 +146,7 @@ feature 'Projected Branches', feature: true, js: true do end wait_for_ajax - expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id) + expect(ProtectedBranch.last.merge_access_levels.first.access_level).to eq(access_type_id) end end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 80f6ebac86c..850b45f84f9 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -228,7 +228,7 @@ describe GitPushService, services: true do execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER) - expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::MASTER) + expect(project.protected_branches.first.merge_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) end it "when pushing a branch for the first time with default branch protection disabled" do @@ -250,7 +250,7 @@ describe GitPushService, services: true do expect(project.protected_branches).not_to be_empty expect(project.protected_branches.last.push_access_level.access_level).to eq(Gitlab::Access::DEVELOPER) - expect(project.protected_branches.last.merge_access_level.access_level).to eq(Gitlab::Access::MASTER) + expect(project.protected_branches.last.merge_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) end it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do @@ -261,7 +261,7 @@ describe GitPushService, services: true do execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER) - expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::DEVELOPER) + expect(project.protected_branches.first.merge_access_levels.first.access_level).to eq(Gitlab::Access::DEVELOPER) end it "when pushing new commits to existing branch" do -- cgit v1.2.1 From 4c28d62672b0f51feede94d9f207f4043c4431f1 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 11:14:45 +0530 Subject: Don't select an access level if already selected. 1. This is in regard to the protected branches feature spec. 2. For example, if "Masters" is already selected, don't re-select "Masters" during the spec. 3. This is due to a bug in the frontend implementation, where selecting an already-selected access level _deselects_ it, which is something we don't need. I'll create a separate issue for this. 4. This hasn't turned up before, because we were manually creating missing access levels prior to e805a64. Now, we just use nested attributes, and missing access levels fail validation. --- spec/features/protected_branches_spec.rb | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb index 5beb658b2f5..709ce7f33b9 100644 --- a/spec/features/protected_branches_spec.rb +++ b/spec/features/protected_branches_spec.rb @@ -93,8 +93,12 @@ feature 'Projected Branches', feature: true, js: true do visit namespace_project_protected_branches_path(project.namespace, project) set_protected_branch_name('master') within('.new_protected_branch') do - find(".js-allowed-to-push").click - within(".dropdown.open .dropdown-menu") { click_on access_type_name } + allowed_to_push_button = find(".js-allowed-to-push") + + unless allowed_to_push_button.text == access_type_name + allowed_to_push_button.click + within(".dropdown.open .dropdown-menu") { click_on access_type_name } + end end click_on "Protect" @@ -124,8 +128,12 @@ feature 'Projected Branches', feature: true, js: true do visit namespace_project_protected_branches_path(project.namespace, project) set_protected_branch_name('master') within('.new_protected_branch') do - find(".js-allowed-to-merge").click - within(".dropdown.open .dropdown-menu") { click_on access_type_name } + allowed_to_merge_button = find(".js-allowed-to-merge") + + unless allowed_to_merge_button.text == access_type_name + allowed_to_merge_button.click + within(".dropdown.open .dropdown-menu") { click_on access_type_name } + end end click_on "Protect" -- cgit v1.2.1 From e9f483355ef07a63d664126c1200762bd1e11271 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 12:00:19 +0530 Subject: Move the "update" portion of the protected branch view into a partial. 1. To improve EE compatibility. --- .../projects/protected_branches/_protected_branch.html.haml | 13 +++---------- .../protected_branches/_update_protected_branch.html.haml | 10 ++++++++++ 2 files changed, 13 insertions(+), 10 deletions(-) create mode 100644 app/views/projects/protected_branches/_update_protected_branch.html.haml diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml index eb4c67daa80..0628134b1bb 100644 --- a/app/views/projects/protected_branches/_protected_branch.html.haml +++ b/app/views/projects/protected_branches/_protected_branch.html.haml @@ -13,16 +13,9 @@ = time_ago_with_tooltip(commit.committed_date) - else (branch was removed from repository) - %td - = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_levels.first.access_level - = dropdown_tag( (protected_branch.merge_access_levels.first.humanize || 'Select') , - options: { toggle_class: 'js-allowed-to-merge', dropdown_class: 'dropdown-menu-selectable js-allowed-to-merge-container', - data: { field_name: "allowed_to_merge_#{protected_branch.id}", access_level_id: protected_branch.merge_access_levels.first.id }}) - %td - = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_levels.first.access_level - = dropdown_tag( (protected_branch.push_access_levels.first.humanize || 'Select') , - options: { toggle_class: 'js-allowed-to-push', dropdown_class: 'dropdown-menu-selectable js-allowed-to-push-container', - data: { field_name: "allowed_to_push_#{protected_branch.id}", access_level_id: protected_branch.push_access_levels.first.id }}) + + = render partial: 'update_protected_branch', locals: { protected_branch: protected_branch } + - if can_admin_project %td = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: 'btn btn-warning' diff --git a/app/views/projects/protected_branches/_update_protected_branch.html.haml b/app/views/projects/protected_branches/_update_protected_branch.html.haml new file mode 100644 index 00000000000..d6044aacaec --- /dev/null +++ b/app/views/projects/protected_branches/_update_protected_branch.html.haml @@ -0,0 +1,10 @@ +%td + = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_levels.first.access_level + = dropdown_tag( (protected_branch.merge_access_levels.first.humanize || 'Select') , + options: { toggle_class: 'js-allowed-to-merge', dropdown_class: 'dropdown-menu-selectable js-allowed-to-merge-container', + data: { field_name: "allowed_to_merge_#{protected_branch.id}", access_level_id: protected_branch.merge_access_levels.first.id }}) +%td + = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_levels.first.access_level + = dropdown_tag( (protected_branch.push_access_levels.first.humanize || 'Select') , + options: { toggle_class: 'js-allowed-to-push', dropdown_class: 'dropdown-menu-selectable js-allowed-to-push-container', + data: { field_name: "allowed_to_push_#{protected_branch.id}", access_level_id: protected_branch.push_access_levels.first.id }}) -- cgit v1.2.1 From 4ddbbcd11a6f03ae36efd4b9016974c34a1465ed Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 12:00:56 +0530 Subject: Improve EE compatibility with protected branch access levels. 1. Change a few incorrect `access_level` to `access_levels.first` that were missed in e805a64. 2. `API::Entities` can iterate over all access levels instead of just the first one. This makes no difference to CE, and makes it more compatible with EE. --- lib/api/entities.rb | 6 ++++-- lib/gitlab/user_access.rb | 4 ++-- spec/services/git_push_service_spec.rb | 6 +++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 7bce427adf6..ec455e67329 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -129,12 +129,14 @@ module API expose :developers_can_push do |repo_branch, options| project = options[:project] - project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_levels.first.access_level == Gitlab::Access::DEVELOPER } + access_levels = project.protected_branches.matching(repo_branch.name).map(&:push_access_levels).flatten + access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER } end expose :developers_can_merge do |repo_branch, options| project = options[:project] - project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_levels.first.access_level == Gitlab::Access::DEVELOPER } + access_levels = project.protected_branches.matching(repo_branch.name).map(&:merge_access_levels).flatten + access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER } end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index c55a7fc4d3d..9858d2e7d83 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -32,7 +32,7 @@ module Gitlab if project.protected_branch?(ref) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) - access_levels = project.protected_branches.matching(ref).map(&:push_access_level) + access_levels = project.protected_branches.matching(ref).map(&:push_access_levels).flatten access_levels.any? { |access_level| access_level.check_access(user) } else user.can?(:push_code, project) @@ -43,7 +43,7 @@ module Gitlab return false unless user if project.protected_branch?(ref) - access_levels = project.protected_branches.matching(ref).map(&:merge_access_level) + access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten access_levels.any? { |access_level| access_level.check_access(user) } else user.can?(:push_code, project) diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 850b45f84f9..7585623b5ef 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -227,7 +227,7 @@ describe GitPushService, services: true do expect(project.default_branch).to eq("master") execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) expect(project.protected_branches).not_to be_empty - expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER) + expect(project.protected_branches.first.push_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) expect(project.protected_branches.first.merge_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) end @@ -249,7 +249,7 @@ describe GitPushService, services: true do execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) expect(project.protected_branches).not_to be_empty - expect(project.protected_branches.last.push_access_level.access_level).to eq(Gitlab::Access::DEVELOPER) + expect(project.protected_branches.last.push_access_levels.first.access_level).to eq(Gitlab::Access::DEVELOPER) expect(project.protected_branches.last.merge_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) end @@ -260,7 +260,7 @@ describe GitPushService, services: true do expect(project.default_branch).to eq("master") execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) expect(project.protected_branches).not_to be_empty - expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER) + expect(project.protected_branches.first.push_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) expect(project.protected_branches.first.merge_access_levels.first.access_level).to eq(Gitlab::Access::DEVELOPER) end -- cgit v1.2.1 From 37651d2f4ce12c16945a5b67360c67768cddb465 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 12:45:48 +0530 Subject: Fix the protected branches factory. 1. Previously, we were using `after_create` to create access levels. 2. At the time of protected branch creation, there are _no_ access levels present, which is invalid, and creation fails. 3. Fixed by setting access levels before the protected branch is created. --- spec/factories/protected_branches.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb index 3b21174987f..42853cac112 100644 --- a/spec/factories/protected_branches.rb +++ b/spec/factories/protected_branches.rb @@ -3,9 +3,9 @@ FactoryGirl.define do name project - after(:create) do |protected_branch| - protected_branch.push_access_levels.create!(access_level: Gitlab::Access::MASTER) - protected_branch.merge_access_levels.create!(access_level: Gitlab::Access::MASTER) + before(:create) do |protected_branch| + protected_branch.push_access_levels.new(access_level: Gitlab::Access::MASTER) + protected_branch.merge_access_levels.new(access_level: Gitlab::Access::MASTER) end trait :developers_can_push do -- cgit v1.2.1 From dd3b738d5b3eb70217d7ac7f9fe441498d2e8e7e Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 13:34:56 +0530 Subject: Fix failing tests relating to backporting ee!581. 1. `GitPushService` was still using `{merge,push}_access_level_attributes` instead of `{merge,push}_access_levels_attributes`. 2. The branches API creates access levels regardless of the state of the `developers_can_{push,merge}` parameters. This is in line with the UI, where Master access is the default for a new protected branch. 3. Use `after(:build)` to create access levels in the `protected_branches` factory, so that `factories_spec` passes. It only builds records, so we need to create access levels on `build` as well. --- app/services/git_push_service.rb | 8 ++++---- lib/api/branches.rb | 29 +++++++++++++++++------------ spec/factories/protected_branches.rb | 2 +- spec/models/project_spec.rb | 6 +++--- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 6f521462cf3..d5fb2018d24 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -91,12 +91,12 @@ class GitPushService < BaseService params = { name: @project.default_branch, - push_access_level_attributes: { + push_access_levels_attributes: [{ access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER - }, - merge_access_level_attributes: { + }], + merge_access_levels_attributes: [{ access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER - } + }] } ProtectedBranches::CreateService.new(@project, current_user, params).execute diff --git a/lib/api/branches.rb b/lib/api/branches.rb index a77afe634f6..b615703df93 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -61,22 +61,27 @@ module API name: @branch.name } - unless developers_can_merge.nil? - protected_branch_params.merge!({ - merge_access_level_attributes: { - access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER - } - }) + # If `developers_can_merge` is switched off, _all_ `DEVELOPER` + # merge_access_levels need to be deleted. + if developers_can_merge == false + protected_branch.merge_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all end - unless developers_can_push.nil? - protected_branch_params.merge!({ - push_access_level_attributes: { - access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER - } - }) + # If `developers_can_push` is switched off, _all_ `DEVELOPER` + # push_access_levels need to be deleted. + if developers_can_push == false + protected_branch.push_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all end + protected_branch_params.merge!( + merge_access_levels_attributes: [{ + access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER + }], + push_access_levels_attributes: [{ + access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER + }] + ) + if protected_branch service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params) service.execute(protected_branch) diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb index 42853cac112..b2695e0482a 100644 --- a/spec/factories/protected_branches.rb +++ b/spec/factories/protected_branches.rb @@ -3,7 +3,7 @@ FactoryGirl.define do name project - before(:create) do |protected_branch| + after(:build) do |protected_branch| protected_branch.push_access_levels.new(access_level: Gitlab::Access::MASTER) protected_branch.merge_access_levels.new(access_level: Gitlab::Access::MASTER) end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 9c3b4712cab..0a32a486703 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1089,13 +1089,13 @@ describe Project, models: true do let(:project) { create(:project) } it 'returns true when the branch matches a protected branch via direct match' do - project.protected_branches.create!(name: 'foo') + create(:protected_branch, project: project, name: "foo") expect(project.protected_branch?('foo')).to eq(true) end it 'returns true when the branch matches a protected branch via wildcard match' do - project.protected_branches.create!(name: 'production/*') + create(:protected_branch, project: project, name: "production/*") expect(project.protected_branch?('production/some-branch')).to eq(true) end @@ -1105,7 +1105,7 @@ describe Project, models: true do end it 'returns false when the branch does not match a protected branch via wildcard match' do - project.protected_branches.create!(name: 'production/*') + create(:protected_branch, project: project, name: "production/*") expect(project.protected_branch?('staging/some-branch')).to eq(false) end -- cgit v1.2.1 From 0e2cecfd628f00e19c33d84f76e2859ebf8a073e Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 14:10:28 +0530 Subject: Fix API::BranchesSpec. - Previously created a protected branch manually, without using a factory. --- spec/requests/api/branches_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index 9444138f93d..3fd989dd7a6 100644 --- a/spec/requests/api/branches_spec.rb +++ b/spec/requests/api/branches_spec.rb @@ -243,7 +243,7 @@ describe API::API, api: true do end it "removes protected branch" do - project.protected_branches.create(name: branch_name) + create(:protected_branch, project: project, name: branch_name) delete api("/projects/#{project.id}/repository/branches/#{branch_name}", user) expect(response).to have_http_status(405) expect(json_response['message']).to eq('Protected branch cant be removed') -- cgit v1.2.1 From 2193ae222b3337f03c18dd7d27408a1b138c2f92 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 15:16:08 +0530 Subject: Backport `AutocompleteController#load_project` from EE!581. - This is an optimization that was made in !581, and it needs to be backported to CE to avoid merge conflicts in the future. --- app/controllers/autocomplete_controller.rb | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index d828d163c28..441030fb545 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -55,11 +55,8 @@ class AutocompleteController < ApplicationController def find_users @users = - if params[:project_id].present? - project = Project.find(params[:project_id]) - return render_404 unless can?(current_user, :read_project, project) - - project.team.users + if @project + @project.team.users elsif params[:group_id].present? group = Group.find(params[:group_id]) return render_404 unless can?(current_user, :read_group, group) @@ -71,4 +68,14 @@ class AutocompleteController < ApplicationController User.none end end + + def load_project + @project ||= begin + if params[:project_id].present? + project = Project.find(params[:project_id]) + return render_404 unless can?(current_user, :read_project, project) + project + end + end + end end -- cgit v1.2.1 From f898a47e146391d869eeade989143513fb5c4ed0 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 15:24:55 +0530 Subject: Fix a missed `before_action` for `AutocompleteController`. - `#find_users` depends on a project being loaded. - Missed adding this in 2193ae222b3337f03c18dd7d27408a1b138c2f92 --- app/controllers/autocomplete_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 441030fb545..e1641ba6265 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -1,5 +1,6 @@ class AutocompleteController < ApplicationController skip_before_action :authenticate_user!, only: [:users] + before_action :load_project, only: [:users] before_action :find_users, only: [:users] def users -- cgit v1.2.1 From b44b09b302e6f4d19c7e6b2dac3be62fe83b31b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila=20Santos?= Date: Tue, 16 Aug 2016 16:14:54 +0000 Subject: Revert "Merge branch '19957-write-tests-for-adding-comments-for-different-line-types-in-diff' into 'master'" This reverts merge request !5417 --- features/project/merge_requests.feature | 10 ++ spec/features/merge_requests/diff_notes_spec.rb | 159 ------------------------ 2 files changed, 10 insertions(+), 159 deletions(-) delete mode 100644 spec/features/merge_requests/diff_notes_spec.rb diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 1b8e4262e40..6bac6011467 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -106,6 +106,16 @@ Feature: Project Merge Requests And I sort the list by "Least popular" Then The list should be sorted by "Least popular" + @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 click on the Changes tab + And I leave a comment like "Line is wrong" on diff + And I switch to the merge request's comments tab + Then I should see a discussion has started on diff + And I should see a badge of "1" next to the discussion link + @javascript Scenario: I see a new comment on merge request diff from another user in the discussion tab Given project "Shop" have "Bug NS-05" open merge request with diffs inside diff --git a/spec/features/merge_requests/diff_notes_spec.rb b/spec/features/merge_requests/diff_notes_spec.rb deleted file mode 100644 index 12e89742b79..00000000000 --- a/spec/features/merge_requests/diff_notes_spec.rb +++ /dev/null @@ -1,159 +0,0 @@ -require 'spec_helper' - -feature 'Diff notes', js: true, feature: true do - include WaitForAjax - - before do - login_as :admin - @merge_request = create(:merge_request) - @project = @merge_request.source_project - end - - context 'merge request diffs' do - let(:comment_button_class) { '.add-diff-note' } - let(:notes_holder_input_class) { 'js-temp-notes-holder' } - let(:notes_holder_input_xpath) { './following-sibling::*[contains(concat(" ", @class, " "), " notes_holder ")]' } - let(:test_note_comment) { 'this is a test note!' } - - context 'when hovering over the parallel view diff file' do - before(:each) do - visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) - click_link 'Side-by-side' - end - - context 'with an old line on the left and no line on the right' do - it 'should allow commenting on the left side' do - should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'left') - end - - it 'should not allow commenting on the right side' do - should_not_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'right') - end - end - - context 'with no line on the left and a new line on the right' do - it 'should not allow commenting on the left side' do - should_not_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'left') - end - - it 'should allow commenting on the right side' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'right') - end - end - - context 'with an old line on the left and a new line on the right' do - it 'should allow commenting on the left side' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'left') - end - - it 'should allow commenting on the right side' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'right') - end - end - - context 'with an unchanged line on the left and an unchanged line on the right' do - it 'should allow commenting on the left side' do - should_allow_commenting(first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..'), 'left') - end - - it 'should allow commenting on the right side' do - should_allow_commenting(first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..'), 'right') - end - end - - context 'with a match line' do - it 'should not allow commenting on the left side' do - should_not_allow_commenting(first('.match').find(:xpath, '..'), 'left') - end - - it 'should not allow commenting on the right side' do - should_not_allow_commenting(first('.match').find(:xpath, '..'), 'right') - end - end - end - - context 'when hovering over the inline view diff file' do - before do - visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) - click_link 'Inline' - end - - context 'with a new line' do - it 'should allow commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) - end - end - - context 'with an old line' do - it 'should allow commenting' do - should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) - end - end - - context 'with an unchanged line' do - it 'should allow commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) - end - end - - context 'with a match line' do - it 'should not allow commenting' do - should_not_allow_commenting(first('.match')) - end - end - end - - def should_allow_commenting(line_holder, diff_side = nil) - line = get_line_components(line_holder, diff_side) - line[:content].hover - expect(line[:num]).to have_css comment_button_class - - comment_on_line(line_holder, line) - wait_for_ajax - - assert_comment_persistence(line_holder) - end - - def should_not_allow_commenting(line_holder, diff_side = nil) - line = get_line_components(line_holder, diff_side) - line[:content].hover - expect(line[:num]).not_to have_css comment_button_class - end - - def get_line_components(line_holder, diff_side = nil) - if diff_side.nil? - get_inline_line_components(line_holder) - else - get_parallel_line_components(line_holder, diff_side) - end - end - - def get_inline_line_components(line_holder) - { content: line_holder.first('.line_content'), num: line_holder.first('.diff-line-num') } - end - - def get_parallel_line_components(line_holder, diff_side = nil) - side_index = diff_side == 'left' ? 0 : 1 - { content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] } - end - - def comment_on_line(line_holder, line) - line[:num].find(comment_button_class).trigger 'click' - expect(line_holder).to have_xpath notes_holder_input_xpath - - notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_input[:class]).to include(notes_holder_input_class) - - notes_holder_input.fill_in 'note[note]', with: test_note_comment - click_button 'Comment' - end - - def assert_comment_persistence(line_holder) - expect(line_holder).to have_xpath notes_holder_input_xpath - - notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_saved[:class]).not_to include(notes_holder_input_class) - expect(notes_holder_saved).to have_content test_note_comment - end - end -end -- cgit v1.2.1 From 8c101fc313208b2256f9b9a2d596a0b398f173e0 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 22:19:49 +0530 Subject: Backport EE assertions in protected branch related specs. - Use assertions in the vein of `merge_access_levels.map(&:access_level)` instead of `merge_access_levels.first.access_level` --- spec/features/protected_branches_spec.rb | 8 ++++---- spec/services/git_push_service_spec.rb | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb index 709ce7f33b9..a0ee6cab7ec 100644 --- a/spec/features/protected_branches_spec.rb +++ b/spec/features/protected_branches_spec.rb @@ -103,7 +103,7 @@ feature 'Projected Branches', feature: true, js: true do click_on "Protect" expect(ProtectedBranch.count).to eq(1) - expect(ProtectedBranch.last.push_access_levels.first.access_level).to eq(access_type_id) + expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to eq([access_type_id]) end it "allows updating protected branches so that #{access_type_name} can push to them" do @@ -119,7 +119,7 @@ feature 'Projected Branches', feature: true, js: true do end wait_for_ajax - expect(ProtectedBranch.last.push_access_levels.first.access_level).to eq(access_type_id) + expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(access_type_id) end end @@ -138,7 +138,7 @@ feature 'Projected Branches', feature: true, js: true do click_on "Protect" expect(ProtectedBranch.count).to eq(1) - expect(ProtectedBranch.last.merge_access_levels.first.access_level).to eq(access_type_id) + expect(ProtectedBranch.last.merge_access_levels.map(&:access_level)).to eq([access_type_id]) end it "allows updating protected branches so that #{access_type_name} can merge to them" do @@ -154,7 +154,7 @@ feature 'Projected Branches', feature: true, js: true do end wait_for_ajax - expect(ProtectedBranch.last.merge_access_levels.first.access_level).to eq(access_type_id) + expect(ProtectedBranch.last.merge_access_levels.map(&:access_level)).to include(access_type_id) end end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 7585623b5ef..6ac1fa8f182 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -227,8 +227,8 @@ describe GitPushService, services: true do expect(project.default_branch).to eq("master") execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) expect(project.protected_branches).not_to be_empty - expect(project.protected_branches.first.push_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) - expect(project.protected_branches.first.merge_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) + expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) + expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) end it "when pushing a branch for the first time with default branch protection disabled" do @@ -249,8 +249,8 @@ describe GitPushService, services: true do execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) expect(project.protected_branches).not_to be_empty - expect(project.protected_branches.last.push_access_levels.first.access_level).to eq(Gitlab::Access::DEVELOPER) - expect(project.protected_branches.last.merge_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) + expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER]) + expect(project.protected_branches.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) end it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do @@ -260,8 +260,8 @@ describe GitPushService, services: true do expect(project.default_branch).to eq("master") execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) expect(project.protected_branches).not_to be_empty - expect(project.protected_branches.first.push_access_levels.first.access_level).to eq(Gitlab::Access::MASTER) - expect(project.protected_branches.first.merge_access_levels.first.access_level).to eq(Gitlab::Access::DEVELOPER) + expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER]) + expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER]) end it "when pushing new commits to existing branch" do -- cgit v1.2.1 From 28726729452ef64270534806e75a9595ea1a659d Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Fri, 24 Jun 2016 16:43:46 -0300 Subject: Load issues and merge requests templates from repository --- CHANGELOG | 1 + app/assets/javascripts/api.js | 51 +++++++----- app/assets/javascripts/application.js | 1 + app/assets/javascripts/blob/template_selector.js | 22 ++++- app/assets/javascripts/dispatcher.js | 2 + .../templates/issuable_template_selector.js.es6 | 51 ++++++++++++ .../templates/issuable_template_selectors.js.es6 | 29 +++++++ app/assets/stylesheets/framework/dropdowns.scss | 7 +- app/assets/stylesheets/pages/issuable.scss | 9 +++ app/controllers/projects/templates_controller.rb | 19 +++++ app/helpers/blob_helper.rb | 41 ++++++++-- app/views/shared/issuable/_filter.html.haml | 2 +- app/views/shared/issuable/_form.html.haml | 24 +++++- config/routes.rb | 5 ++ doc/workflow/README.md | 1 + doc/workflow/description_templates.md | 12 +++ doc/workflow/img/description_templates.png | Bin 0 -> 57670 bytes lib/api/templates.rb | 26 +++--- lib/gitlab/template/base_template.rb | 71 ++++++++++------ .../template/finders/base_template_finder.rb | 35 ++++++++ .../template/finders/global_template_finder.rb | 38 +++++++++ .../template/finders/repo_template_finder.rb | 59 ++++++++++++++ lib/gitlab/template/gitignore.rb | 22 ----- lib/gitlab/template/gitignore_template.rb | 26 ++++++ lib/gitlab/template/gitlab_ci_yml.rb | 27 ------- lib/gitlab/template/gitlab_ci_yml_template.rb | 31 +++++++ lib/gitlab/template/issue_template.rb | 19 +++++ lib/gitlab/template/merge_request_template.rb | 19 +++++ .../projects/templates_controller_spec.rb | 48 +++++++++++ spec/features/projects/issuable_templates_spec.rb | 89 +++++++++++++++++++++ spec/lib/gitlab/template/gitignore_spec.rb | 40 --------- .../lib/gitlab/template/gitignore_template_spec.rb | 40 +++++++++ .../gitlab/template/gitlab_ci_yml_template_spec.rb | 41 ++++++++++ spec/lib/gitlab/template/issue_template_spec.rb | 89 +++++++++++++++++++++ .../gitlab/template/merge_request_template_spec.rb | 89 +++++++++++++++++++++ spec/requests/api/templates_spec.rb | 65 ++++++++------- 36 files changed, 960 insertions(+), 191 deletions(-) create mode 100644 app/assets/javascripts/templates/issuable_template_selector.js.es6 create mode 100644 app/assets/javascripts/templates/issuable_template_selectors.js.es6 create mode 100644 app/controllers/projects/templates_controller.rb create mode 100644 doc/workflow/description_templates.md create mode 100644 doc/workflow/img/description_templates.png create mode 100644 lib/gitlab/template/finders/base_template_finder.rb create mode 100644 lib/gitlab/template/finders/global_template_finder.rb create mode 100644 lib/gitlab/template/finders/repo_template_finder.rb delete mode 100644 lib/gitlab/template/gitignore.rb create mode 100644 lib/gitlab/template/gitignore_template.rb delete mode 100644 lib/gitlab/template/gitlab_ci_yml.rb create mode 100644 lib/gitlab/template/gitlab_ci_yml_template.rb create mode 100644 lib/gitlab/template/issue_template.rb create mode 100644 lib/gitlab/template/merge_request_template.rb create mode 100644 spec/controllers/projects/templates_controller_spec.rb create mode 100644 spec/features/projects/issuable_templates_spec.rb delete mode 100644 spec/lib/gitlab/template/gitignore_spec.rb create mode 100644 spec/lib/gitlab/template/gitignore_template_spec.rb create mode 100644 spec/lib/gitlab/template/gitlab_ci_yml_template_spec.rb create mode 100644 spec/lib/gitlab/template/issue_template_spec.rb create mode 100644 spec/lib/gitlab/template/merge_request_template_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 9299639a3ab..aececed9add 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -40,6 +40,7 @@ v 8.11.0 (unreleased) - Various redundant database indexes have been removed - Update `timeago` plugin to use multiple string/locale settings - Remove unused images (ClemMakesApps) + - Get issue and merge request description templates from repositories - Limit git rev-list output count to one in forced push check - Show deployment status on merge requests with external URLs - Clean up unused routes (Josef Strzibny) diff --git a/app/assets/javascripts/api.js b/app/assets/javascripts/api.js index 49c2ac0dac3..84b292e59c6 100644 --- a/app/assets/javascripts/api.js +++ b/app/assets/javascripts/api.js @@ -9,10 +9,11 @@ licensePath: "/api/:version/licenses/:key", gitignorePath: "/api/:version/gitignores/:key", gitlabCiYmlPath: "/api/:version/gitlab_ci_ymls/:key", + issuableTemplatePath: "/:namespace_path/:project_path/templates/:type/:key", + group: function(group_id, callback) { - var url; - url = Api.buildUrl(Api.groupPath); - url = url.replace(':id', group_id); + var url = Api.buildUrl(Api.groupPath) + .replace(':id', group_id); return $.ajax({ url: url, data: { @@ -24,8 +25,7 @@ }); }, groups: function(query, skip_ldap, callback) { - var url; - url = Api.buildUrl(Api.groupsPath); + var url = Api.buildUrl(Api.groupsPath); return $.ajax({ url: url, data: { @@ -39,8 +39,7 @@ }); }, namespaces: function(query, callback) { - var url; - url = Api.buildUrl(Api.namespacesPath); + var url = Api.buildUrl(Api.namespacesPath); return $.ajax({ url: url, data: { @@ -54,8 +53,7 @@ }); }, projects: function(query, order, callback) { - var url; - url = Api.buildUrl(Api.projectsPath); + var url = Api.buildUrl(Api.projectsPath); return $.ajax({ url: url, data: { @@ -70,9 +68,8 @@ }); }, newLabel: function(project_id, data, callback) { - var url; - url = Api.buildUrl(Api.labelsPath); - url = url.replace(':id', project_id); + var url = Api.buildUrl(Api.labelsPath) + .replace(':id', project_id); data.private_token = gon.api_token; return $.ajax({ url: url, @@ -86,9 +83,8 @@ }); }, groupProjects: function(group_id, query, callback) { - var url; - url = Api.buildUrl(Api.groupProjectsPath); - url = url.replace(':id', group_id); + var url = Api.buildUrl(Api.groupProjectsPath) + .replace(':id', group_id); return $.ajax({ url: url, data: { @@ -102,8 +98,8 @@ }); }, licenseText: function(key, data, callback) { - var url; - url = Api.buildUrl(Api.licensePath).replace(':key', key); + var url = Api.buildUrl(Api.licensePath) + .replace(':key', key); return $.ajax({ url: url, data: data @@ -112,19 +108,32 @@ }); }, gitignoreText: function(key, callback) { - var url; - url = Api.buildUrl(Api.gitignorePath).replace(':key', key); + var url = Api.buildUrl(Api.gitignorePath) + .replace(':key', key); return $.get(url, function(gitignore) { return callback(gitignore); }); }, gitlabCiYml: function(key, callback) { - var url; - url = Api.buildUrl(Api.gitlabCiYmlPath).replace(':key', key); + var url = Api.buildUrl(Api.gitlabCiYmlPath) + .replace(':key', key); return $.get(url, function(file) { return callback(file); }); }, + issueTemplate: function(namespacePath, projectPath, key, type, callback) { + var url = Api.buildUrl(Api.issuableTemplatePath) + .replace(':key', key) + .replace(':type', type) + .replace(':project_path', projectPath) + .replace(':namespace_path', namespacePath); + $.ajax({ + url: url, + dataType: 'json' + }).done(function(file) { + callback(null, file); + }).error(callback); + }, buildUrl: function(url) { if (gon.relative_url_root != null) { url = gon.relative_url_root + url; diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index f1aab067351..e596b98603b 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -41,6 +41,7 @@ /*= require date.format */ /*= require_directory ./behaviors */ /*= require_directory ./blob */ +/*= require_directory ./templates */ /*= require_directory ./commit */ /*= require_directory ./extensions */ /*= require_directory ./lib/utils */ diff --git a/app/assets/javascripts/blob/template_selector.js b/app/assets/javascripts/blob/template_selector.js index 2cf0a6631b8..b0a37ef0e0a 100644 --- a/app/assets/javascripts/blob/template_selector.js +++ b/app/assets/javascripts/blob/template_selector.js @@ -9,6 +9,7 @@ } this.onClick = bind(this.onClick, this); this.dropdown = opts.dropdown, this.data = opts.data, this.pattern = opts.pattern, this.wrapper = opts.wrapper, this.editor = opts.editor, this.fileEndpoint = opts.fileEndpoint, this.$input = (ref = opts.$input) != null ? ref : $('#file_name'); + this.dropdownIcon = $('.fa-chevron-down', this.dropdown); this.buildDropdown(); this.bindEvents(); this.onFilenameUpdate(); @@ -60,11 +61,26 @@ return this.requestFile(item); }; - TemplateSelector.prototype.requestFile = function(item) {}; + TemplateSelector.prototype.requestFile = function(item) { + // This `requestFile` method is an abstract method that should + // be added by all subclasses. + }; - TemplateSelector.prototype.requestFileSuccess = function(file) { + TemplateSelector.prototype.requestFileSuccess = function(file, skipFocus) { this.editor.setValue(file.content, 1); - return this.editor.focus(); + if (!skipFocus) this.editor.focus(); + }; + + TemplateSelector.prototype.startLoadingSpinner = function() { + this.dropdownIcon + .addClass('fa-spinner fa-spin') + .removeClass('fa-chevron-down'); + }; + + TemplateSelector.prototype.stopLoadingSpinner = function() { + this.dropdownIcon + .addClass('fa-chevron-down') + .removeClass('fa-spinner fa-spin'); }; return TemplateSelector; diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 3946e861976..7160fa71ce5 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -55,6 +55,7 @@ shortcut_handler = new ShortcutsNavigation(); new GLForm($('.issue-form')); new IssuableForm($('.issue-form')); + new IssuableTemplateSelectors(); break; case 'projects:merge_requests:new': case 'projects:merge_requests:edit': @@ -62,6 +63,7 @@ shortcut_handler = new ShortcutsNavigation(); new GLForm($('.merge-request-form')); new IssuableForm($('.merge-request-form')); + new IssuableTemplateSelectors(); break; case 'projects:tags:new': new ZenMode(); diff --git a/app/assets/javascripts/templates/issuable_template_selector.js.es6 b/app/assets/javascripts/templates/issuable_template_selector.js.es6 new file mode 100644 index 00000000000..c32ddf80219 --- /dev/null +++ b/app/assets/javascripts/templates/issuable_template_selector.js.es6 @@ -0,0 +1,51 @@ +/*= require ../blob/template_selector */ + +((global) => { + class IssuableTemplateSelector extends TemplateSelector { + constructor(...args) { + super(...args); + this.projectPath = this.dropdown.data('project-path'); + this.namespacePath = this.dropdown.data('namespace-path'); + this.issuableType = this.wrapper.data('issuable-type'); + this.titleInput = $(`#${this.issuableType}_title`); + + let initialQuery = { + name: this.dropdown.data('selected') + }; + + if (initialQuery.name) this.requestFile(initialQuery); + + $('.reset-template', this.dropdown.parent()).on('click', () => { + if (this.currentTemplate) this.setInputValueToTemplateContent(); + }); + } + + requestFile(query) { + this.startLoadingSpinner(); + Api.issueTemplate(this.namespacePath, this.projectPath, query.name, this.issuableType, (err, currentTemplate) => { + this.currentTemplate = currentTemplate; + if (err) return; // Error handled by global AJAX error handler + this.stopLoadingSpinner(); + this.setInputValueToTemplateContent(); + }); + return; + } + + setInputValueToTemplateContent() { + // `this.requestFileSuccess` sets the value of the description input field + // to the content of the template selected. + if (this.titleInput.val() === '') { + // If the title has not yet been set, focus the title input and + // skip focusing the description input by setting `true` as the 2nd + // argument to `requestFileSuccess`. + this.requestFileSuccess(this.currentTemplate, true); + this.titleInput.focus(); + } else { + this.requestFileSuccess(this.currentTemplate); + } + return; + } + } + + global.IssuableTemplateSelector = IssuableTemplateSelector; +})(window); diff --git a/app/assets/javascripts/templates/issuable_template_selectors.js.es6 b/app/assets/javascripts/templates/issuable_template_selectors.js.es6 new file mode 100644 index 00000000000..bd8cdde033e --- /dev/null +++ b/app/assets/javascripts/templates/issuable_template_selectors.js.es6 @@ -0,0 +1,29 @@ +((global) => { + class IssuableTemplateSelectors { + constructor(opts = {}) { + this.$dropdowns = opts.$dropdowns || $('.js-issuable-selector'); + this.editor = opts.editor || this.initEditor(); + + this.$dropdowns.each((i, dropdown) => { + let $dropdown = $(dropdown); + new IssuableTemplateSelector({ + pattern: /(\.md)/, + data: $dropdown.data('data'), + wrapper: $dropdown.closest('.js-issuable-selector-wrap'), + dropdown: $dropdown, + editor: this.editor + }); + }); + } + + initEditor() { + let editor = $('.markdown-area'); + // Proxy ace-editor's .setValue to jQuery's .val + editor.setValue = editor.val; + editor.getValue = editor.val; + return editor; + } + } + + global.IssuableTemplateSelectors = IssuableTemplateSelectors; +})(window); diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index e8eafa15899..f1635a53763 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -56,9 +56,13 @@ position: absolute; top: 50%; right: 6px; - margin-top: -4px; + margin-top: -6px; color: $dropdown-toggle-icon-color; font-size: 10px; + &.fa-spinner { + font-size: 16px; + margin-top: -8px; + } } &:hover, { @@ -406,6 +410,7 @@ font-size: 14px; a { + cursor: pointer; padding-left: 10px; } } diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 7a50bc9c832..46c4a11aa2e 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -395,3 +395,12 @@ display: inline-block; line-height: 18px; } + +.js-issuable-selector-wrap { + .js-issuable-selector { + width: 100%; + } + @media (max-width: $screen-sm-max) { + margin-bottom: $gl-padding; + } +} diff --git a/app/controllers/projects/templates_controller.rb b/app/controllers/projects/templates_controller.rb new file mode 100644 index 00000000000..694b468c8d3 --- /dev/null +++ b/app/controllers/projects/templates_controller.rb @@ -0,0 +1,19 @@ +class Projects::TemplatesController < Projects::ApplicationController + before_action :authenticate_user!, :get_template_class + + def show + template = @template_type.find(params[:key], project) + + respond_to do |format| + format.json { render json: template.to_json } + end + end + + private + + def get_template_class + template_types = { issue: Gitlab::Template::IssueTemplate, merge_request: Gitlab::Template::MergeRequestTemplate }.with_indifferent_access + @template_type = template_types[params[:template_type]] + render json: [], status: 404 unless @template_type + end +end diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 48c27828219..1cb5d847626 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -182,17 +182,42 @@ module BlobHelper } end + def selected_template(issuable) + templates = issuable_templates(issuable) + params[:issuable_template] if templates.include?(params[:issuable_template]) + end + + def can_add_template?(issuable) + names = issuable_templates(issuable) + names.empty? && can?(current_user, :push_code, @project) && !@project.private? + end + + def merge_request_template_names + @merge_request_templates ||= Gitlab::Template::MergeRequestTemplate.dropdown_names(ref_project) + end + + def issue_template_names + @issue_templates ||= Gitlab::Template::IssueTemplate.dropdown_names(ref_project) + end + + def issuable_templates(issuable) + @issuable_templates ||= + if issuable.is_a?(Issue) + issue_template_names + elsif issuable.is_a?(MergeRequest) + merge_request_template_names + end + end + + def ref_project + @ref_project ||= @target_project || @project + end + def gitignore_names - @gitignore_names ||= - Gitlab::Template::Gitignore.categories.keys.map do |k| - [k, Gitlab::Template::Gitignore.by_category(k).map { |t| { name: t.name } }] - end.to_h + @gitignore_names ||= Gitlab::Template::GitignoreTemplate.dropdown_names end def gitlab_ci_ymls - @gitlab_ci_ymls ||= - Gitlab::Template::GitlabCiYml.categories.keys.map do |k| - [k, Gitlab::Template::GitlabCiYml.by_category(k).map { |t| { name: t.name } }] - end.to_h + @gitlab_ci_ymls ||= Gitlab::Template::GitlabCiYmlTemplate.dropdown_names end end diff --git a/app/views/shared/issuable/_filter.html.haml b/app/views/shared/issuable/_filter.html.haml index 0b7fa8c7d06..c957cd84479 100644 --- a/app/views/shared/issuable/_filter.html.haml +++ b/app/views/shared/issuable/_filter.html.haml @@ -45,7 +45,7 @@ .filter-item.inline = dropdown_tag("Milestone", options: { title: "Assign milestone", toggle_class: 'js-milestone-select js-extra-options js-filter-submit js-filter-bulk-update', filter: true, dropdown_class: "dropdown-menu-selectable dropdown-menu-milestone", placeholder: "Search milestones", data: { show_no: true, field_name: "update[milestone_id]", project_id: @project.id, milestones: namespace_project_milestones_path(@project.namespace, @project, :json), use_id: true } }) .filter-item.inline.labels-filter - = render "shared/issuable/label_dropdown", classes: ['js-filter-bulk-update', 'js-multiselect'], show_create: false, show_footer: false, extra_options: false, filter_submit: false, show_footer: false, data_options: { persist_when_hide: "true", field_name: "update[label_ids][]", show_no: false, show_any: false, use_id: true } + = render "shared/issuable/label_dropdown", classes: ['js-filter-bulk-update', 'js-multiselect'], show_create: false, show_footer: false, extra_options: false, filter_submit: false, data_options: { persist_when_hide: "true", field_name: "update[label_ids][]", show_no: false, show_any: false, use_id: true } .filter-item.inline = dropdown_tag("Subscription", options: { toggle_class: "js-subscription-event", title: "Change subscription", dropdown_class: "dropdown-menu-selectable", data: { field_name: "update[subscription_event]" } } ) do %ul diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index c30bdb0ae91..210b43c7e0b 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -2,7 +2,22 @@ .form-group = f.label :title, class: 'control-label' - .col-sm-10 + + - issuable_template_names = issuable_templates(issuable) + + - if issuable_template_names.any? + .col-sm-3.col-lg-2 + .js-issuable-selector-wrap{ data: { issuable_type: issuable.class.to_s.underscore.downcase } } + - title = selected_template(issuable) || "Choose a template" + + = dropdown_tag(title, options: { toggle_class: 'js-issuable-selector', + title: title, filter: true, placeholder: 'Filter', footer_content: true, + data: { data: issuable_template_names, field_name: 'issuable_template', selected: selected_template(issuable), project_path: @project.path, namespace_path: @project.namespace.path } } ) do + %ul.dropdown-footer-list + %li + %a.reset-template + Reset template + %div{ class: issuable_template_names.any? ? 'col-sm-7 col-lg-8' : 'col-sm-10' } = f.text_field :title, maxlength: 255, autofocus: true, autocomplete: 'off', class: 'form-control pad', required: true @@ -23,6 +38,13 @@ to prevent a %strong Work In Progress merge request from being merged before it's ready. + + - if can_add_template?(issuable) + %p.help-block + Add + = link_to "issuable templates", help_page_path('workflow/description_templates') + to help your contributors communicate effectively! + .form-group.detail-page-description = f.label :description, 'Description', class: 'control-label' .col-sm-10 diff --git a/config/routes.rb b/config/routes.rb index 1d2db91344f..63a8827a6a2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -528,6 +528,11 @@ Rails.application.routes.draw do put '/update/*id', to: 'blob#update', constraints: { id: /.+/ }, as: 'update_blob' post '/preview/*id', to: 'blob#preview', constraints: { id: /.+/ }, as: 'preview_blob' + # + # Templates + # + get '/templates/:template_type/:key' => 'templates#show', as: :template + scope do get( '/blob/*id/diff', diff --git a/doc/workflow/README.md b/doc/workflow/README.md index 49dec613716..993349e5b46 100644 --- a/doc/workflow/README.md +++ b/doc/workflow/README.md @@ -17,6 +17,7 @@ - [Share projects with other groups](share_projects_with_other_groups.md) - [Web Editor](web_editor.md) - [Releases](releases.md) +- [Issuable Templates](issuable_templates.md) - [Milestones](milestones.md) - [Merge Requests](merge_requests.md) - [Revert changes](revert_changes.md) diff --git a/doc/workflow/description_templates.md b/doc/workflow/description_templates.md new file mode 100644 index 00000000000..9514564af02 --- /dev/null +++ b/doc/workflow/description_templates.md @@ -0,0 +1,12 @@ +# Description templates + +Description templates allow you to define context-specific templates for issue and merge request description fields for your project. When in use, users that create a new issue or merge request can select a description template to help them communicate with other contributors effectively. + +Every GitLab project can define its own set of description templates as they are added to the root directory of a GitLab project's repository. + +Description templates are written in markdown _(`.md`)_ and stored in your projects repository under the `/.gitlab/issue_templates/` and `/.gitlab/merge_request_templates/` directories. + +![Description templates](img/description_templates.png) + +_Example:_ +`/.gitlab/issue_templates/bug.md` will enable the `bug` dropdown option for new issues. When `bug` is selected, the content from the `bug.md` template file will be copied to the issue description field. diff --git a/doc/workflow/img/description_templates.png b/doc/workflow/img/description_templates.png new file mode 100644 index 00000000000..af2e9403826 Binary files /dev/null and b/doc/workflow/img/description_templates.png differ diff --git a/lib/api/templates.rb b/lib/api/templates.rb index 18408797756..b9e718147e1 100644 --- a/lib/api/templates.rb +++ b/lib/api/templates.rb @@ -1,21 +1,28 @@ module API class Templates < Grape::API - TEMPLATE_TYPES = { - gitignores: Gitlab::Template::Gitignore, - gitlab_ci_ymls: Gitlab::Template::GitlabCiYml + GLOBAL_TEMPLATE_TYPES = { + gitignores: Gitlab::Template::GitignoreTemplate, + gitlab_ci_ymls: Gitlab::Template::GitlabCiYmlTemplate }.freeze - TEMPLATE_TYPES.each do |template, klass| + helpers do + def render_response(template_type, template) + not_found!(template_type.to_s.singularize) unless template + present template, with: Entities::Template + end + end + + GLOBAL_TEMPLATE_TYPES.each do |template_type, klass| # Get the list of the available template # # Example Request: # GET /gitignores # GET /gitlab_ci_ymls - get template.to_s do + get template_type.to_s do present klass.all, with: Entities::TemplatesList end - # Get the text for a specific template + # Get the text for a specific template present in local filesystem # # Parameters: # name (required) - The name of a template @@ -23,13 +30,10 @@ module API # Example Request: # GET /gitignores/Elixir # GET /gitlab_ci_ymls/Ruby - get "#{template}/:name" do + get "#{template_type}/:name" do required_attributes! [:name] - new_template = klass.find(params[:name]) - not_found!(template.to_s.singularize) unless new_template - - present new_template, with: Entities::Template + render_response(template_type, new_template) end end end diff --git a/lib/gitlab/template/base_template.rb b/lib/gitlab/template/base_template.rb index 760ff3e614a..7ebec8e2cff 100644 --- a/lib/gitlab/template/base_template.rb +++ b/lib/gitlab/template/base_template.rb @@ -1,8 +1,9 @@ module Gitlab module Template class BaseTemplate - def initialize(path) + def initialize(path, project = nil) @path = path + @finder = self.class.finder(project) end def name @@ -10,23 +11,32 @@ module Gitlab end def content - File.read(@path) + @finder.read(@path) + end + + def to_json + { name: name, content: content } end class << self - def all - self.categories.keys.flat_map { |cat| by_category(cat) } + def all(project = nil) + if categories.any? + categories.keys.flat_map { |cat| by_category(cat, project) } + else + by_category("", project) + end end - def find(key) - file_name = "#{key}#{self.extension}" - - directory = select_directory(file_name) - directory ? new(File.join(category_directory(directory), file_name)) : nil + def find(key, project = nil) + path = self.finder(project).find(key) + path.present? ? new(path, project) : nil end + # Set categories as sub directories + # Example: { "category_name_1" => "directory_path_1", "category_name_2" => "directory_name_2" } + # Default is no category with all files in base dir of each class def categories - raise NotImplementedError + {} end def extension @@ -37,29 +47,40 @@ module Gitlab raise NotImplementedError end - def by_category(category) - templates_for_directory(category_directory(category)) + # Defines which strategy will be used to get templates files + # RepoTemplateFinder - Finds templates on project repository, templates are filtered perproject + # GlobalTemplateFinder - Finds templates on gitlab installation source, templates can be used in all projects + def finder(project = nil) + raise NotImplementedError end - def category_directory(category) - File.join(base_dir, categories[category]) + def by_category(category, project = nil) + directory = category_directory(category) + files = finder(project).list_files_for(directory) + + files.map { |f| new(f, project) } end - private + def category_directory(category) + return base_dir unless category.present? - def select_directory(file_name) - categories.keys.find do |category| - File.exist?(File.join(category_directory(category), file_name)) - end + File.join(base_dir, categories[category]) end - def templates_for_directory(dir) - dir << '/' unless dir.end_with?('/') - Dir.glob(File.join(dir, "*#{self.extension}")).select { |f| f =~ filter_regex }.map { |f| new(f) } - end + # If template is organized by category it returns { category_name: [{ name: template_name }, { name: template2_name }] } + # If no category is present returns [{ name: template_name }, { name: template2_name}] + def dropdown_names(project = nil) + return [] if project && !project.repository.exists? - def filter_regex - @filter_reges ||= /#{Regexp.escape(extension)}\z/ + if categories.any? + categories.keys.map do |category| + files = self.by_category(category, project) + [category, files.map { |t| { name: t.name } }] + end.to_h + else + files = self.all(project) + files.map { |t| { name: t.name } } + end end end end diff --git a/lib/gitlab/template/finders/base_template_finder.rb b/lib/gitlab/template/finders/base_template_finder.rb new file mode 100644 index 00000000000..473b05257c6 --- /dev/null +++ b/lib/gitlab/template/finders/base_template_finder.rb @@ -0,0 +1,35 @@ +module Gitlab + module Template + module Finders + class BaseTemplateFinder + def initialize(base_dir) + @base_dir = base_dir + end + + def list_files_for + raise NotImplementedError + end + + def read + raise NotImplementedError + end + + def find + raise NotImplementedError + end + + def category_directory(category) + return @base_dir unless category.present? + + @base_dir + @categories[category] + end + + class << self + def filter_regex(extension) + /#{Regexp.escape(extension)}\z/ + end + end + end + end + end +end diff --git a/lib/gitlab/template/finders/global_template_finder.rb b/lib/gitlab/template/finders/global_template_finder.rb new file mode 100644 index 00000000000..831da45191f --- /dev/null +++ b/lib/gitlab/template/finders/global_template_finder.rb @@ -0,0 +1,38 @@ +# Searches and reads file present on Gitlab installation directory +module Gitlab + module Template + module Finders + class GlobalTemplateFinder < BaseTemplateFinder + def initialize(base_dir, extension, categories = {}) + @categories = categories + @extension = extension + super(base_dir) + end + + def read(path) + File.read(path) + end + + def find(key) + file_name = "#{key}#{@extension}" + + directory = select_directory(file_name) + directory ? File.join(category_directory(directory), file_name) : nil + end + + def list_files_for(dir) + dir << '/' unless dir.end_with?('/') + Dir.glob(File.join(dir, "*#{@extension}")).select { |f| f =~ self.class.filter_regex(@extension) } + end + + private + + def select_directory(file_name) + @categories.keys.find do |category| + File.exist?(File.join(category_directory(category), file_name)) + end + end + end + end + end +end diff --git a/lib/gitlab/template/finders/repo_template_finder.rb b/lib/gitlab/template/finders/repo_template_finder.rb new file mode 100644 index 00000000000..22c39436cb2 --- /dev/null +++ b/lib/gitlab/template/finders/repo_template_finder.rb @@ -0,0 +1,59 @@ +# Searches and reads files present on each Gitlab project repository +module Gitlab + module Template + module Finders + class RepoTemplateFinder < BaseTemplateFinder + # Raised when file is not found + class FileNotFoundError < StandardError; end + + def initialize(project, base_dir, extension, categories = {}) + @categories = categories + @extension = extension + @repository = project.repository + @commit = @repository.head_commit if @repository.exists? + + super(base_dir) + end + + def read(path) + blob = @repository.blob_at(@commit.id, path) if @commit + raise FileNotFoundError if blob.nil? + blob.data + end + + def find(key) + file_name = "#{key}#{@extension}" + directory = select_directory(file_name) + raise FileNotFoundError if directory.nil? + + category_directory(directory) + file_name + end + + def list_files_for(dir) + return [] unless @commit + + dir << '/' unless dir.end_with?('/') + + entries = @repository.tree(:head, dir).entries + + names = entries.map(&:name) + names.select { |f| f =~ self.class.filter_regex(@extension) } + end + + private + + def select_directory(file_name) + return [] unless @commit + + # Insert root as directory + directories = ["", @categories.keys] + + directories.find do |category| + path = category_directory(category) + file_name + @repository.blob_at(@commit.id, path) + end + end + end + end + end +end diff --git a/lib/gitlab/template/gitignore.rb b/lib/gitlab/template/gitignore.rb deleted file mode 100644 index 964fbfd4de3..00000000000 --- a/lib/gitlab/template/gitignore.rb +++ /dev/null @@ -1,22 +0,0 @@ -module Gitlab - module Template - class Gitignore < BaseTemplate - class << self - def extension - '.gitignore' - end - - def categories - { - "Languages" => '', - "Global" => 'Global' - } - end - - def base_dir - Rails.root.join('vendor/gitignore') - end - end - end - end -end diff --git a/lib/gitlab/template/gitignore_template.rb b/lib/gitlab/template/gitignore_template.rb new file mode 100644 index 00000000000..8d2a9d2305c --- /dev/null +++ b/lib/gitlab/template/gitignore_template.rb @@ -0,0 +1,26 @@ +module Gitlab + module Template + class GitignoreTemplate < BaseTemplate + class << self + def extension + '.gitignore' + end + + def categories + { + "Languages" => '', + "Global" => 'Global' + } + end + + def base_dir + Rails.root.join('vendor/gitignore') + end + + def finder(project = nil) + Gitlab::Template::Finders::GlobalTemplateFinder.new(self.base_dir, self.extension, self.categories) + end + end + end + end +end diff --git a/lib/gitlab/template/gitlab_ci_yml.rb b/lib/gitlab/template/gitlab_ci_yml.rb deleted file mode 100644 index 7f480fe33c0..00000000000 --- a/lib/gitlab/template/gitlab_ci_yml.rb +++ /dev/null @@ -1,27 +0,0 @@ -module Gitlab - module Template - class GitlabCiYml < BaseTemplate - def content - explanation = "# This file is a template, and might need editing before it works on your project." - [explanation, super].join("\n") - end - - class << self - def extension - '.gitlab-ci.yml' - end - - def categories - { - "General" => '', - "Pages" => 'Pages' - } - end - - def base_dir - Rails.root.join('vendor/gitlab-ci-yml') - end - end - end - end -end diff --git a/lib/gitlab/template/gitlab_ci_yml_template.rb b/lib/gitlab/template/gitlab_ci_yml_template.rb new file mode 100644 index 00000000000..8d1a1ed54c9 --- /dev/null +++ b/lib/gitlab/template/gitlab_ci_yml_template.rb @@ -0,0 +1,31 @@ +module Gitlab + module Template + class GitlabCiYmlTemplate < BaseTemplate + def content + explanation = "# This file is a template, and might need editing before it works on your project." + [explanation, super].join("\n") + end + + class << self + def extension + '.gitlab-ci.yml' + end + + def categories + { + "General" => '', + "Pages" => 'Pages' + } + end + + def base_dir + Rails.root.join('vendor/gitlab-ci-yml') + end + + def finder(project = nil) + Gitlab::Template::Finders::GlobalTemplateFinder.new(self.base_dir, self.extension, self.categories) + end + end + end + end +end diff --git a/lib/gitlab/template/issue_template.rb b/lib/gitlab/template/issue_template.rb new file mode 100644 index 00000000000..c6fa8d3eafc --- /dev/null +++ b/lib/gitlab/template/issue_template.rb @@ -0,0 +1,19 @@ +module Gitlab + module Template + class IssueTemplate < BaseTemplate + class << self + def extension + '.md' + end + + def base_dir + '.gitlab/issue_templates/' + end + + def finder(project) + Gitlab::Template::Finders::RepoTemplateFinder.new(project, self.base_dir, self.extension, self.categories) + end + end + end + end +end diff --git a/lib/gitlab/template/merge_request_template.rb b/lib/gitlab/template/merge_request_template.rb new file mode 100644 index 00000000000..f826c02f3b5 --- /dev/null +++ b/lib/gitlab/template/merge_request_template.rb @@ -0,0 +1,19 @@ +module Gitlab + module Template + class MergeRequestTemplate < BaseTemplate + class << self + def extension + '.md' + end + + def base_dir + '.gitlab/merge_request_templates/' + end + + def finder(project) + Gitlab::Template::Finders::RepoTemplateFinder.new(project, self.base_dir, self.extension, self.categories) + end + end + end + end +end diff --git a/spec/controllers/projects/templates_controller_spec.rb b/spec/controllers/projects/templates_controller_spec.rb new file mode 100644 index 00000000000..7b3a26d7ca7 --- /dev/null +++ b/spec/controllers/projects/templates_controller_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper' + +describe Projects::TemplatesController do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:user2) { create(:user) } + let(:file_path_1) { '.gitlab/issue_templates/bug.md' } + let(:body) { JSON.parse(response.body) } + + before do + project.team << [user, :developer] + sign_in(user) + end + + before do + project.team.add_user(user, Gitlab::Access::MASTER) + project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) + end + + describe '#show' do + it 'renders template name and content as json' do + get(:show, namespace_id: project.namespace.to_param, template_type: "issue", key: "bug", project_id: project.path, format: :json) + + expect(response.status).to eq(200) + expect(body["name"]).to eq("bug") + expect(body["content"]).to eq("something valid") + end + + it 'renders 404 when unauthorized' do + sign_in(user2) + get(:show, namespace_id: project.namespace.to_param, template_type: "issue", key: "bug", project_id: project.path, format: :json) + + expect(response.status).to eq(404) + end + + it 'renders 404 when template type is not found' do + sign_in(user) + get(:show, namespace_id: project.namespace.to_param, template_type: "dont_exist", key: "bug", project_id: project.path, format: :json) + + expect(response.status).to eq(404) + end + + it 'renders 404 without errors' do + sign_in(user) + expect { get(:show, namespace_id: project.namespace.to_param, template_type: "dont_exist", key: "bug", project_id: project.path, format: :json) }.not_to raise_error + end + end +end diff --git a/spec/features/projects/issuable_templates_spec.rb b/spec/features/projects/issuable_templates_spec.rb new file mode 100644 index 00000000000..4a83740621a --- /dev/null +++ b/spec/features/projects/issuable_templates_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +feature 'issuable templates', feature: true, js: true do + include WaitForAjax + + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + + before do + project.team << [user, :master] + login_as user + end + + context 'user creates an issue using templates' do + let(:template_content) { 'this is a test "bug" template' } + let(:issue) { create(:issue, author: user, assignee: user, project: project) } + + background do + project.repository.commit_file(user, '.gitlab/issue_templates/bug.md', template_content, 'added issue template', 'master', false) + visit edit_namespace_project_issue_path project.namespace, project, issue + fill_in :'issue[title]', with: 'test issue title' + end + + scenario 'user selects "bug" template' do + select_template 'bug' + wait_for_ajax + preview_template + save_changes + end + end + + context 'user creates a merge request using templates' do + let(:template_content) { 'this is a test "feature-proposal" template' } + let(:merge_request) { create(:merge_request, :with_diffs, source_project: project) } + + background do + project.repository.commit_file(user, '.gitlab/merge_request_templates/feature-proposal.md', template_content, 'added merge request template', 'master', false) + visit edit_namespace_project_merge_request_path project.namespace, project, merge_request + fill_in :'merge_request[title]', with: 'test merge request title' + end + + scenario 'user selects "feature-proposal" template' do + select_template 'feature-proposal' + wait_for_ajax + preview_template + save_changes + end + end + + context 'user creates a merge request from a forked project using templates' do + let(:template_content) { 'this is a test "feature-proposal" template' } + let(:fork_user) { create(:user) } + let(:fork_project) { create(:project, :public) } + let(:merge_request) { create(:merge_request, :with_diffs, source_project: fork_project) } + + background do + logout + project.team << [fork_user, :developer] + fork_project.team << [fork_user, :master] + create(:forked_project_link, forked_to_project: fork_project, forked_from_project: project) + login_as fork_user + fork_project.repository.commit_file(fork_user, '.gitlab/merge_request_templates/feature-proposal.md', template_content, 'added merge request template', 'master', false) + visit edit_namespace_project_merge_request_path fork_project.namespace, fork_project, merge_request + fill_in :'merge_request[title]', with: 'test merge request title' + end + + scenario 'user selects "feature-proposal" template' do + select_template 'feature-proposal' + wait_for_ajax + preview_template + save_changes + end + end + + def preview_template + click_link 'Preview' + expect(page).to have_content template_content + end + + def save_changes + click_button "Save changes" + expect(page).to have_content template_content + end + + def select_template(name) + first('.js-issuable-selector').click + first('.js-issuable-selector-wrap .dropdown-content a', text: name).click + end +end diff --git a/spec/lib/gitlab/template/gitignore_spec.rb b/spec/lib/gitlab/template/gitignore_spec.rb deleted file mode 100644 index bc0ec9325cc..00000000000 --- a/spec/lib/gitlab/template/gitignore_spec.rb +++ /dev/null @@ -1,40 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Template::Gitignore do - subject { described_class } - - describe '.all' do - it 'strips the gitignore suffix' do - expect(subject.all.first.name).not_to end_with('.gitignore') - end - - it 'combines the globals and rest' do - all = subject.all.map(&:name) - - expect(all).to include('Vim') - expect(all).to include('Ruby') - end - end - - describe '.find' do - it 'returns nil if the file does not exist' do - expect(subject.find('mepmep-yadida')).to be nil - end - - it 'returns the Gitignore object of a valid file' do - ruby = subject.find('Ruby') - - expect(ruby).to be_a Gitlab::Template::Gitignore - expect(ruby.name).to eq('Ruby') - end - end - - describe '#content' do - it 'loads the full file' do - gitignore = subject.new(Rails.root.join('vendor/gitignore/Ruby.gitignore')) - - expect(gitignore.name).to eq 'Ruby' - expect(gitignore.content).to start_with('*.gem') - end - end -end diff --git a/spec/lib/gitlab/template/gitignore_template_spec.rb b/spec/lib/gitlab/template/gitignore_template_spec.rb new file mode 100644 index 00000000000..9750a012e22 --- /dev/null +++ b/spec/lib/gitlab/template/gitignore_template_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe Gitlab::Template::GitignoreTemplate do + subject { described_class } + + describe '.all' do + it 'strips the gitignore suffix' do + expect(subject.all.first.name).not_to end_with('.gitignore') + end + + it 'combines the globals and rest' do + all = subject.all.map(&:name) + + expect(all).to include('Vim') + expect(all).to include('Ruby') + end + end + + describe '.find' do + it 'returns nil if the file does not exist' do + expect(subject.find('mepmep-yadida')).to be nil + end + + it 'returns the Gitignore object of a valid file' do + ruby = subject.find('Ruby') + + expect(ruby).to be_a Gitlab::Template::GitignoreTemplate + expect(ruby.name).to eq('Ruby') + end + end + + describe '#content' do + it 'loads the full file' do + gitignore = subject.new(Rails.root.join('vendor/gitignore/Ruby.gitignore')) + + expect(gitignore.name).to eq 'Ruby' + expect(gitignore.content).to start_with('*.gem') + end + end +end diff --git a/spec/lib/gitlab/template/gitlab_ci_yml_template_spec.rb b/spec/lib/gitlab/template/gitlab_ci_yml_template_spec.rb new file mode 100644 index 00000000000..e3b8321eda3 --- /dev/null +++ b/spec/lib/gitlab/template/gitlab_ci_yml_template_spec.rb @@ -0,0 +1,41 @@ +require 'spec_helper' + +describe Gitlab::Template::GitlabCiYmlTemplate do + subject { described_class } + + describe '.all' do + it 'strips the gitlab-ci suffix' do + expect(subject.all.first.name).not_to end_with('.gitlab-ci.yml') + end + + it 'combines the globals and rest' do + all = subject.all.map(&:name) + + expect(all).to include('Elixir') + expect(all).to include('Docker') + expect(all).to include('Ruby') + end + end + + describe '.find' do + it 'returns nil if the file does not exist' do + expect(subject.find('mepmep-yadida')).to be nil + end + + it 'returns the GitlabCiYml object of a valid file' do + ruby = subject.find('Ruby') + + expect(ruby).to be_a Gitlab::Template::GitlabCiYmlTemplate + expect(ruby.name).to eq('Ruby') + end + end + + describe '#content' do + it 'loads the full file' do + gitignore = subject.new(Rails.root.join('vendor/gitlab-ci-yml/Ruby.gitlab-ci.yml')) + + expect(gitignore.name).to eq 'Ruby' + expect(gitignore.content).to start_with('#') + end + end +end diff --git a/spec/lib/gitlab/template/issue_template_spec.rb b/spec/lib/gitlab/template/issue_template_spec.rb new file mode 100644 index 00000000000..f770857e958 --- /dev/null +++ b/spec/lib/gitlab/template/issue_template_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +describe Gitlab::Template::IssueTemplate do + subject { described_class } + + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:file_path_1) { '.gitlab/issue_templates/bug.md' } + let(:file_path_2) { '.gitlab/issue_templates/template_test.md' } + let(:file_path_3) { '.gitlab/issue_templates/feature_proposal.md' } + + before do + project.team.add_user(user, Gitlab::Access::MASTER) + project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) + project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false) + project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false) + end + + describe '.all' do + it 'strips the md suffix' do + expect(subject.all(project).first.name).not_to end_with('.issue_template') + end + + it 'combines the globals and rest' do + all = subject.all(project).map(&:name) + + expect(all).to include('bug') + expect(all).to include('feature_proposal') + expect(all).to include('template_test') + end + end + + describe '.find' do + it 'returns nil if the file does not exist' do + expect { subject.find('mepmep-yadida', project) }.to raise_error(Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError) + end + + it 'returns the issue object of a valid file' do + ruby = subject.find('bug', project) + + expect(ruby).to be_a Gitlab::Template::IssueTemplate + expect(ruby.name).to eq('bug') + end + end + + describe '.by_category' do + it 'return array of templates' do + all = subject.by_category('', project).map(&:name) + expect(all).to include('bug') + expect(all).to include('feature_proposal') + expect(all).to include('template_test') + end + + context 'when repo is bare or empty' do + let(:empty_project) { create(:empty_project) } + before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + + it "returns empty array" do + templates = subject.by_category('', empty_project) + expect(templates).to be_empty + end + end + end + + describe '#content' do + it 'loads the full file' do + issue_template = subject.new('.gitlab/issue_templates/bug.md', project) + + expect(issue_template.name).to eq 'bug' + expect(issue_template.content).to eq('something valid') + end + + it 'raises error when file is not found' do + issue_template = subject.new('.gitlab/issue_templates/bugnot.md', project) + expect { issue_template.content }.to raise_error(Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError) + end + + context "when repo is empty" do + let(:empty_project) { create(:empty_project) } + + before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + + it "raises file not found" do + issue_template = subject.new('.gitlab/issue_templates/not_existent.md', empty_project) + expect { issue_template.content }.to raise_error(Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError) + end + end + end +end diff --git a/spec/lib/gitlab/template/merge_request_template_spec.rb b/spec/lib/gitlab/template/merge_request_template_spec.rb new file mode 100644 index 00000000000..bb0f68043fa --- /dev/null +++ b/spec/lib/gitlab/template/merge_request_template_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +describe Gitlab::Template::MergeRequestTemplate do + subject { described_class } + + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:file_path_1) { '.gitlab/merge_request_templates/bug.md' } + let(:file_path_2) { '.gitlab/merge_request_templates/template_test.md' } + let(:file_path_3) { '.gitlab/merge_request_templates/feature_proposal.md' } + + before do + project.team.add_user(user, Gitlab::Access::MASTER) + project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) + project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false) + project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false) + end + + describe '.all' do + it 'strips the md suffix' do + expect(subject.all(project).first.name).not_to end_with('.issue_template') + end + + it 'combines the globals and rest' do + all = subject.all(project).map(&:name) + + expect(all).to include('bug') + expect(all).to include('feature_proposal') + expect(all).to include('template_test') + end + end + + describe '.find' do + it 'returns nil if the file does not exist' do + expect { subject.find('mepmep-yadida', project) }.to raise_error(Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError) + end + + it 'returns the merge request object of a valid file' do + ruby = subject.find('bug', project) + + expect(ruby).to be_a Gitlab::Template::MergeRequestTemplate + expect(ruby.name).to eq('bug') + end + end + + describe '.by_category' do + it 'return array of templates' do + all = subject.by_category('', project).map(&:name) + expect(all).to include('bug') + expect(all).to include('feature_proposal') + expect(all).to include('template_test') + end + + context 'when repo is bare or empty' do + let(:empty_project) { create(:empty_project) } + before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + + it "returns empty array" do + templates = subject.by_category('', empty_project) + expect(templates).to be_empty + end + end + end + + describe '#content' do + it 'loads the full file' do + issue_template = subject.new('.gitlab/merge_request_templates/bug.md', project) + + expect(issue_template.name).to eq 'bug' + expect(issue_template.content).to eq('something valid') + end + + it 'raises error when file is not found' do + issue_template = subject.new('.gitlab/merge_request_templates/bugnot.md', project) + expect { issue_template.content }.to raise_error(Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError) + end + + context "when repo is empty" do + let(:empty_project) { create(:empty_project) } + + before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + + it "raises file not found" do + issue_template = subject.new('.gitlab/merge_request_templates/not_existent.md', empty_project) + expect { issue_template.content }.to raise_error(Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError) + end + end + end +end diff --git a/spec/requests/api/templates_spec.rb b/spec/requests/api/templates_spec.rb index 68d0f41b489..5bd5b861792 100644 --- a/spec/requests/api/templates_spec.rb +++ b/spec/requests/api/templates_spec.rb @@ -3,50 +3,53 @@ require 'spec_helper' describe API::Templates, api: true do include ApiHelpers - describe 'the Template Entity' do - before { get api('/gitignores/Ruby') } + context 'global templates' do + describe 'the Template Entity' do + before { get api('/gitignores/Ruby') } - it { expect(json_response['name']).to eq('Ruby') } - it { expect(json_response['content']).to include('*.gem') } - end + it { expect(json_response['name']).to eq('Ruby') } + it { expect(json_response['content']).to include('*.gem') } + end - describe 'the TemplateList Entity' do - before { get api('/gitignores') } + describe 'the TemplateList Entity' do + before { get api('/gitignores') } - it { expect(json_response.first['name']).not_to be_nil } - it { expect(json_response.first['content']).to be_nil } - end + it { expect(json_response.first['name']).not_to be_nil } + it { expect(json_response.first['content']).to be_nil } + end - context 'requesting gitignores' do - describe 'GET /gitignores' do - it 'returns a list of available gitignore templates' do - get api('/gitignores') + context 'requesting gitignores' do + describe 'GET /gitignores' do + it 'returns a list of available gitignore templates' do + get api('/gitignores') - expect(response).to have_http_status(200) - expect(json_response).to be_an Array - expect(json_response.size).to be > 15 + expect(response.status).to eq(200) + expect(json_response).to be_an Array + expect(json_response.size).to be > 15 + end end end - end - context 'requesting gitlab-ci-ymls' do - describe 'GET /gitlab_ci_ymls' do - it 'returns a list of available gitlab_ci_ymls' do - get api('/gitlab_ci_ymls') + context 'requesting gitlab-ci-ymls' do + describe 'GET /gitlab_ci_ymls' do + it 'returns a list of available gitlab_ci_ymls' do + get api('/gitlab_ci_ymls') - expect(response).to have_http_status(200) - expect(json_response).to be_an Array - expect(json_response.first['name']).not_to be_nil + expect(response.status).to eq(200) + expect(json_response).to be_an Array + expect(json_response.first['name']).not_to be_nil + end end end - end - describe 'GET /gitlab_ci_ymls/Ruby' do - it 'adds a disclaimer on the top' do - get api('/gitlab_ci_ymls/Ruby') + describe 'GET /gitlab_ci_ymls/Ruby' do + it 'adds a disclaimer on the top' do + get api('/gitlab_ci_ymls/Ruby') - expect(response).to have_http_status(200) - expect(json_response['content']).to start_with("# This file is a template,") + expect(response).to have_http_status(200) + expect(json_response['name']).not_to be_nil + expect(json_response['content']).to start_with("# This file is a template,") + end end end end -- cgit v1.2.1