From 454d227b45a563cb21ea8fa5091e687a2876380f Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 11 Aug 2015 01:44:40 -0400 Subject: Remove all permission checking from Reference filters --- lib/gitlab/markdown/cross_project_reference.rb | 11 +---- lib/gitlab/markdown/user_reference_filter.rb | 6 --- .../markdown/commit_range_reference_filter_spec.rb | 50 +++++++------------ .../markdown/commit_reference_filter_spec.rb | 46 +++++++----------- .../markdown/cross_project_reference_spec.rb | 12 ----- .../gitlab/markdown/issue_reference_filter_spec.rb | 56 ++++++++-------------- .../merge_request_reference_filter_spec.rb | 46 +++++++----------- .../markdown/snippet_reference_filter_spec.rb | 44 ++++++----------- .../gitlab/markdown/user_reference_filter_spec.rb | 48 +++++++------------ spec/support/filter_spec_helper.rb | 14 ------ 10 files changed, 105 insertions(+), 228 deletions(-) diff --git a/lib/gitlab/markdown/cross_project_reference.rb b/lib/gitlab/markdown/cross_project_reference.rb index 855748fdccc..6ab04a584b0 100644 --- a/lib/gitlab/markdown/cross_project_reference.rb +++ b/lib/gitlab/markdown/cross_project_reference.rb @@ -13,18 +13,11 @@ module Gitlab # # ref - String reference. # - # Returns a Project, or nil if the reference can't be accessed + # Returns a Project, or nil if the reference can't be found def project_from_ref(ref) return context[:project] unless ref - other = Project.find_with_namespace(ref) - return nil unless other && user_can_reference_project?(other) - - other - end - - def user_can_reference_project?(project, user = context[:current_user]) - Ability.abilities.allowed?(user, :read_project, project) + Project.find_with_namespace(ref) end end end diff --git a/lib/gitlab/markdown/user_reference_filter.rb b/lib/gitlab/markdown/user_reference_filter.rb index 1871e52df0e..0446cd49f8e 100644 --- a/lib/gitlab/markdown/user_reference_filter.rb +++ b/lib/gitlab/markdown/user_reference_filter.rb @@ -80,8 +80,6 @@ module Gitlab end def link_to_group(group, namespace) - return unless user_can_reference_group?(namespace) - push_result(:user, *namespace.users) url = urls.group_url(group, only_path: context[:only_path]) @@ -100,10 +98,6 @@ module Gitlab text = User.reference_prefix + user %(#{text}) end - - def user_can_reference_group?(group) - Ability.abilities.allowed?(context[:current_user], :read_group, group) - end end end end diff --git a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb index 3c6c84a0416..6813d6db14c 100644 --- a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb @@ -106,45 +106,31 @@ module Gitlab::Markdown range.project = project2 end - context 'when user can access reference' do - before { allow_cross_reference! } - - it 'links to a valid reference' do - doc = filter("See #{reference}") - - expect(doc.css('a').first.attr('href')). - to eq urls.namespace_project_compare_url(project2.namespace, project2, range.to_param) - end - - it 'links with adjacent text' do - doc = filter("Fixed (#{reference}.)") - - exp = Regexp.escape("#{project2.to_reference}@#{range.to_s}") - expect(doc.to_html).to match(/\(#{exp}<\/a>\.\)/) - end + it 'links to a valid reference' do + doc = filter("See #{reference}") - it 'ignores invalid commit IDs on the referenced project' do - exp = act = "Fixed #{project2.to_reference}@#{commit1.id.reverse}...#{commit2.id}" - expect(filter(act).to_html).to eq exp + expect(doc.css('a').first.attr('href')). + to eq urls.namespace_project_compare_url(project2.namespace, project2, range.to_param) + end - exp = act = "Fixed #{project2.to_reference}@#{commit1.id}...#{commit2.id.reverse}" - expect(filter(act).to_html).to eq exp - end + it 'links with adjacent text' do + doc = filter("Fixed (#{reference}.)") - it 'adds to the results hash' do - result = pipeline_result("See #{reference}") - expect(result[:references][:commit_range]).not_to be_empty - end + exp = Regexp.escape("#{project2.to_reference}@#{range.to_s}") + expect(doc.to_html).to match(/\(#{exp}<\/a>\.\)/) end - context 'when user cannot access reference' do - before { disallow_cross_reference! } + it 'ignores invalid commit IDs on the referenced project' do + exp = act = "Fixed #{project2.to_reference}@#{commit1.id.reverse}...#{commit2.id}" + expect(filter(act).to_html).to eq exp - it 'ignores valid references' do - exp = act = "See #{reference}" + exp = act = "Fixed #{project2.to_reference}@#{commit1.id}...#{commit2.id.reverse}" + expect(filter(act).to_html).to eq exp + end - expect(filter(act).to_html).to eq exp - end + it 'adds to the results hash' do + result = pipeline_result("See #{reference}") + expect(result[:references][:commit_range]).not_to be_empty end end end diff --git a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb index 9ed438252b3..f937b4f50ee 100644 --- a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb @@ -99,42 +99,28 @@ module Gitlab::Markdown let(:commit) { project2.commit } let(:reference) { commit.to_reference(project) } - context 'when user can access reference' do - before { allow_cross_reference! } - - it 'links to a valid reference' do - doc = filter("See #{reference}") - - expect(doc.css('a').first.attr('href')). - to eq urls.namespace_project_commit_url(project2.namespace, project2, commit.id) - end - - it 'links with adjacent text' do - doc = filter("Fixed (#{reference}.)") + it 'links to a valid reference' do + doc = filter("See #{reference}") - exp = Regexp.escape(project2.to_reference) - expect(doc.to_html).to match(/\(#{exp}@#{commit.short_id}<\/a>\.\)/) - end + expect(doc.css('a').first.attr('href')). + to eq urls.namespace_project_commit_url(project2.namespace, project2, commit.id) + end - it 'ignores invalid commit IDs on the referenced project' do - exp = act = "Committed #{invalidate_reference(reference)}" - expect(filter(act).to_html).to eq exp - end + it 'links with adjacent text' do + doc = filter("Fixed (#{reference}.)") - it 'adds to the results hash' do - result = pipeline_result("See #{reference}") - expect(result[:references][:commit]).not_to be_empty - end + exp = Regexp.escape(project2.to_reference) + expect(doc.to_html).to match(/\(#{exp}@#{commit.short_id}<\/a>\.\)/) end - context 'when user cannot access reference' do - before { disallow_cross_reference! } - - it 'ignores valid references' do - exp = act = "See #{reference}" + it 'ignores invalid commit IDs on the referenced project' do + exp = act = "Committed #{invalidate_reference(reference)}" + expect(filter(act).to_html).to eq exp + end - expect(filter(act).to_html).to eq exp - end + it 'adds to the results hash' do + result = pipeline_result("See #{reference}") + expect(result[:references][:commit]).not_to be_empty end end end diff --git a/spec/lib/gitlab/markdown/cross_project_reference_spec.rb b/spec/lib/gitlab/markdown/cross_project_reference_spec.rb index 4698d6138c2..6490d6f7a42 100644 --- a/spec/lib/gitlab/markdown/cross_project_reference_spec.rb +++ b/spec/lib/gitlab/markdown/cross_project_reference_spec.rb @@ -35,21 +35,9 @@ module Gitlab::Markdown context 'and the user has permission to read it' do it 'returns the referenced project' do - expect(self).to receive(:user_can_reference_project?). - with(project2).and_return(true) - expect(project_from_ref('cross/reference')).to eq project2 end end - - context 'and the user does not have permission to read it' do - it 'returns nil' do - expect(self).to receive(:user_can_reference_project?). - with(project2).and_return(false) - - expect(project_from_ref('cross/reference')).to be_nil - end - end end end end diff --git a/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb b/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb index 1dd54f58588..96787954516 100644 --- a/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb @@ -96,49 +96,35 @@ module Gitlab::Markdown let(:issue) { create(:issue, project: project2) } let(:reference) { issue.to_reference(project) } - context 'when user can access reference' do - before { allow_cross_reference! } + it 'ignores valid references when cross-reference project uses external tracker' do + expect_any_instance_of(Project).to receive(:get_issue). + with(issue.iid).and_return(nil) - it 'ignores valid references when cross-reference project uses external tracker' do - expect_any_instance_of(Project).to receive(:get_issue). - with(issue.iid).and_return(nil) - - exp = act = "Issue #{reference}" - expect(filter(act).to_html).to eq exp - end - - it 'links to a valid reference' do - doc = filter("See #{reference}") - - expect(doc.css('a').first.attr('href')). - to eq helper.url_for_issue(issue.iid, project2) - end - - it 'links with adjacent text' do - doc = filter("Fixed (#{reference}.)") - expect(doc.to_html).to match(/\(#{Regexp.escape(reference)}<\/a>\.\)/) - end + exp = act = "Issue #{reference}" + expect(filter(act).to_html).to eq exp + end - it 'ignores invalid issue IDs on the referenced project' do - exp = act = "Fixed #{invalidate_reference(reference)}" + it 'links to a valid reference' do + doc = filter("See #{reference}") - expect(filter(act).to_html).to eq exp - end + expect(doc.css('a').first.attr('href')). + to eq helper.url_for_issue(issue.iid, project2) + end - it 'adds to the results hash' do - result = pipeline_result("Fixed #{reference}") - expect(result[:references][:issue]).to eq [issue] - end + it 'links with adjacent text' do + doc = filter("Fixed (#{reference}.)") + expect(doc.to_html).to match(/\(#{Regexp.escape(reference)}<\/a>\.\)/) end - context 'when user cannot access reference' do - before { disallow_cross_reference! } + it 'ignores invalid issue IDs on the referenced project' do + exp = act = "Fixed #{invalidate_reference(reference)}" - it 'ignores valid references' do - exp = act = "See #{reference}" + expect(filter(act).to_html).to eq exp + end - expect(filter(act).to_html).to eq exp - end + it 'adds to the results hash' do + result = pipeline_result("Fixed #{reference}") + expect(result[:references][:issue]).to eq [issue] end end end diff --git a/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb b/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb index 66616b93368..feba08f7200 100644 --- a/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb @@ -84,42 +84,28 @@ module Gitlab::Markdown let(:merge) { create(:merge_request, source_project: project2) } let(:reference) { merge.to_reference(project) } - context 'when user can access reference' do - before { allow_cross_reference! } - - it 'links to a valid reference' do - doc = filter("See #{reference}") - - expect(doc.css('a').first.attr('href')). - to eq urls.namespace_project_merge_request_url(project2.namespace, - project, merge) - end - - it 'links with adjacent text' do - doc = filter("Merge (#{reference}.)") - expect(doc.to_html).to match(/\(#{Regexp.escape(reference)}<\/a>\.\)/) - end - - it 'ignores invalid merge IDs on the referenced project' do - exp = act = "Merge #{invalidate_reference(reference)}" + it 'links to a valid reference' do + doc = filter("See #{reference}") - expect(filter(act).to_html).to eq exp - end + expect(doc.css('a').first.attr('href')). + to eq urls.namespace_project_merge_request_url(project2.namespace, + project, merge) + end - it 'adds to the results hash' do - result = pipeline_result("Merge #{reference}") - expect(result[:references][:merge_request]).to eq [merge] - end + it 'links with adjacent text' do + doc = filter("Merge (#{reference}.)") + expect(doc.to_html).to match(/\(#{Regexp.escape(reference)}<\/a>\.\)/) end - context 'when user cannot access reference' do - before { disallow_cross_reference! } + it 'ignores invalid merge IDs on the referenced project' do + exp = act = "Merge #{invalidate_reference(reference)}" - it 'ignores valid references' do - exp = act = "See #{reference}" + expect(filter(act).to_html).to eq exp + end - expect(filter(act).to_html).to eq exp - end + it 'adds to the results hash' do + result = pipeline_result("Merge #{reference}") + expect(result[:references][:merge_request]).to eq [merge] end end end diff --git a/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb b/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb index fd3f0d20fad..02d581a7c46 100644 --- a/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb @@ -83,41 +83,27 @@ module Gitlab::Markdown let(:snippet) { create(:project_snippet, project: project2) } let(:reference) { snippet.to_reference(project) } - context 'when user can access reference' do - before { allow_cross_reference! } - - it 'links to a valid reference' do - doc = filter("See #{reference}") - - expect(doc.css('a').first.attr('href')). - to eq urls.namespace_project_snippet_url(project2.namespace, project2, snippet) - end - - it 'links with adjacent text' do - doc = filter("See (#{reference}.)") - expect(doc.to_html).to match(/\(#{Regexp.escape(reference)}<\/a>\.\)/) - end - - it 'ignores invalid snippet IDs on the referenced project' do - exp = act = "See #{invalidate_reference(reference)}" + it 'links to a valid reference' do + doc = filter("See #{reference}") - expect(filter(act).to_html).to eq exp - end + expect(doc.css('a').first.attr('href')). + to eq urls.namespace_project_snippet_url(project2.namespace, project2, snippet) + end - it 'adds to the results hash' do - result = pipeline_result("Snippet #{reference}") - expect(result[:references][:snippet]).to eq [snippet] - end + it 'links with adjacent text' do + doc = filter("See (#{reference}.)") + expect(doc.to_html).to match(/\(#{Regexp.escape(reference)}<\/a>\.\)/) end - context 'when user cannot access reference' do - before { disallow_cross_reference! } + it 'ignores invalid snippet IDs on the referenced project' do + exp = act = "See #{invalidate_reference(reference)}" - it 'ignores valid references' do - exp = act = "See #{reference}" + expect(filter(act).to_html).to eq exp + end - expect(filter(act).to_html).to eq exp - end + it 'adds to the results hash' do + result = pipeline_result("Snippet #{reference}") + expect(result[:references][:snippet]).to eq [snippet] end end end diff --git a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb index b2155fab59b..d3028cd3b88 100644 --- a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb @@ -83,40 +83,26 @@ module Gitlab::Markdown let(:user) { create(:user) } let(:reference) { group.to_reference } - context 'that the current user can read' do - before do - group.add_developer(user) - end - - it 'links to the Group' do - doc = filter("Hey #{reference}", current_user: user) - expect(doc.css('a').first.attr('href')).to eq urls.group_url(group) - end - - it 'includes a data-group-id attribute' do - doc = filter("Hey #{reference}", current_user: user) - link = doc.css('a').first - - expect(link).to have_attribute('data-group-id') - expect(link.attr('data-group-id')).to eq group.id.to_s - end - - it 'adds to the results hash' do - result = pipeline_result("Hey #{reference}", current_user: user) - expect(result[:references][:user]).to eq group.users - end + before do + group.add_developer(user) + end + + it 'links to the Group' do + doc = filter("Hey #{reference}", current_user: user) + expect(doc.css('a').first.attr('href')).to eq urls.group_url(group) end - context 'that the current user cannot read' do - it 'ignores references to the Group' do - doc = filter("Hey #{reference}", current_user: user) - expect(doc.to_html).to eq "Hey #{reference}" - end + it 'includes a data-group-id attribute' do + doc = filter("Hey #{reference}", current_user: user) + link = doc.css('a').first - it 'does not add to the results hash' do - result = pipeline_result("Hey #{reference}", current_user: user) - expect(result[:references][:user]).to eq [] - end + expect(link).to have_attribute('data-group-id') + expect(link.attr('data-group-id')).to eq group.id.to_s + end + + it 'adds to the results hash' do + result = pipeline_result("Hey #{reference}", current_user: user) + expect(result[:references][:user]).to eq group.users end end diff --git a/spec/support/filter_spec_helper.rb b/spec/support/filter_spec_helper.rb index 755964e9a3d..25df3896291 100644 --- a/spec/support/filter_spec_helper.rb +++ b/spec/support/filter_spec_helper.rb @@ -55,20 +55,6 @@ module FilterSpecHelper end end - # Stub CrossProjectReference#user_can_reference_project? to return true for - # the current test - def allow_cross_reference! - allow_any_instance_of(described_class). - to receive(:user_can_reference_project?).and_return(true) - end - - # Stub CrossProjectReference#user_can_reference_project? to return false for - # the current test - def disallow_cross_reference! - allow_any_instance_of(described_class). - to receive(:user_can_reference_project?).and_return(false) - end - # Shortcut to Rails' auto-generated routes helpers, to avoid including the # module def urls -- cgit v1.2.1 From 7f75300573ff8f8e610bf4b0a3b4f2832552a1e9 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 4 Aug 2015 22:25:08 -0400 Subject: Add RedactorFilter --- lib/gitlab/markdown.rb | 1 + lib/gitlab/markdown/redactor_filter.rb | 66 ++++++++++++++++++++ spec/lib/gitlab/markdown/redactor_filter_spec.rb | 76 ++++++++++++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 lib/gitlab/markdown/redactor_filter.rb create mode 100644 spec/lib/gitlab/markdown/redactor_filter_spec.rb diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 097caf67a65..e07fdb702fc 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -41,6 +41,7 @@ module Gitlab autoload :IssueReferenceFilter, 'gitlab/markdown/issue_reference_filter' autoload :LabelReferenceFilter, 'gitlab/markdown/label_reference_filter' autoload :MergeRequestReferenceFilter, 'gitlab/markdown/merge_request_reference_filter' + autoload :RedactorFilter, 'gitlab/markdown/redactor_filter' autoload :RelativeLinkFilter, 'gitlab/markdown/relative_link_filter' autoload :SanitizationFilter, 'gitlab/markdown/sanitization_filter' autoload :SnippetReferenceFilter, 'gitlab/markdown/snippet_reference_filter' diff --git a/lib/gitlab/markdown/redactor_filter.rb b/lib/gitlab/markdown/redactor_filter.rb new file mode 100644 index 00000000000..9faee4567ae --- /dev/null +++ b/lib/gitlab/markdown/redactor_filter.rb @@ -0,0 +1,66 @@ +require 'gitlab/markdown' +require 'html/pipeline/filter' + +module Gitlab + module Markdown + # HTML filter that removes references to records that the current user does + # not have permission to view. + # + # Expected to be run in its own post-processing pipeline. + # + class RedactorFilter < HTML::Pipeline::Filter + def call + doc.css('a.gfm').each do |node| + unless user_can_reference?(node) + node.replace(node.text) + end + end + + doc + end + + def user_can_reference?(node) + if node.has_attribute?('data-group-id') + user_can_reference_group?(node.attr('data-group-id')) + elsif node.has_attribute?('data-project-id') + user_can_reference_project?(node.attr('data-project-id')) + elsif node.has_attribute?('data-user-id') + user_can_reference_user?(node.attr('data-user-id')) + else + false + end + end + + def user_can_reference_group?(id) + group = Group.find(id) + + group && can?(:read_group, group) + end + + def user_can_reference_project?(id) + project = Project.find(id) + + project && can?(:read_project, project) + end + + def user_can_reference_user?(id) + # Permit all user reference links + true + end + + private + + def abilities + Ability.abilities + end + + def can?(ability, object) + abilities.allowed?(current_user, ability, object) + end + + def current_user + context[:current_user] + end + end + end +end diff --git a/spec/lib/gitlab/markdown/redactor_filter_spec.rb b/spec/lib/gitlab/markdown/redactor_filter_spec.rb new file mode 100644 index 00000000000..a6cf3c64236 --- /dev/null +++ b/spec/lib/gitlab/markdown/redactor_filter_spec.rb @@ -0,0 +1,76 @@ +require 'spec_helper' + +module Gitlab::Markdown + describe RedactorFilter do + include ActionView::Helpers::UrlHelper + include FilterSpecHelper + + it 'ignores non-GFM links' do + html = %(See Google) + doc = filter(html, current_user: double) + + expect(doc.css('a').length).to eq 1 + end + + def reference_link(data) + link_to('text', '', class: 'gfm', data: data) + end + + context 'with data-group-id' do + it 'removes unpermitted Group references' do + user = create(:user) + group = create(:group) + + link = reference_link(group_id: group.id) + doc = filter(link, current_user: user) + + expect(doc.css('a').length).to eq 0 + end + + it 'allows permitted Group references' do + user = create(:user) + group = create(:group) + group.add_developer(user) + + link = reference_link(group_id: group.id) + doc = filter(link, current_user: user) + + expect(doc.css('a').length).to eq 1 + end + end + + context 'with data-project-id' do + it 'removes unpermitted Project references' do + user = create(:user) + project = create(:empty_project) + + link = reference_link(project_id: project.id) + doc = filter(link, current_user: user) + + expect(doc.css('a').length).to eq 0 + end + + it 'allows permitted Project references' do + user = create(:user) + project = create(:empty_project) + project.team << [user, :master] + + link = reference_link(project_id: project.id) + doc = filter(link, current_user: user) + + expect(doc.css('a').length).to eq 1 + end + end + + context 'with data-user-id' do + it 'allows any User reference' do + user = create(:user) + + link = reference_link(user_id: user.id) + doc = filter(link) + + expect(doc.css('a').length).to eq 1 + end + end + end +end -- cgit v1.2.1 From 1ec68ea9c208317b6d981da4bf7d022ccd62bb3e Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 1 Sep 2015 17:43:35 -0400 Subject: Default `user_can_reference?` to true when no attributes match Now, a reference link with a `.gfm` class but without one of our `data-*-id` attributes should be shown to the user rather than hidden. --- lib/gitlab/markdown/redactor_filter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/markdown/redactor_filter.rb b/lib/gitlab/markdown/redactor_filter.rb index 9faee4567ae..92925f866a9 100644 --- a/lib/gitlab/markdown/redactor_filter.rb +++ b/lib/gitlab/markdown/redactor_filter.rb @@ -27,7 +27,7 @@ module Gitlab elsif node.has_attribute?('data-user-id') user_can_reference_user?(node.attr('data-user-id')) else - false + true end end -- cgit v1.2.1 From 8db4628b8a736e85b2f40c007f07b43f780065a6 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 1 Sep 2015 17:45:25 -0400 Subject: Rescue from `RecordNotFound` in RedactorFilter --- lib/gitlab/markdown/redactor_filter.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/gitlab/markdown/redactor_filter.rb b/lib/gitlab/markdown/redactor_filter.rb index 92925f866a9..ae1c3c365bd 100644 --- a/lib/gitlab/markdown/redactor_filter.rb +++ b/lib/gitlab/markdown/redactor_filter.rb @@ -35,12 +35,16 @@ module Gitlab group = Group.find(id) group && can?(:read_group, group) + rescue ActiveRecord::RecordNotFound + false end def user_can_reference_project?(id) project = Project.find(id) project && can?(:read_project, project) + rescue ActiveRecord::RecordNotFound + false end def user_can_reference_user?(id) -- cgit v1.2.1 From 81fc63bec042d4b8eb7ed3522393204f3b9a7a53 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 1 Sep 2015 17:46:36 -0400 Subject: Remove `current_user` context from markdown and gfm helpers These helpers are no longer dependent on the current user state. Hooray! --- app/helpers/gitlab_markdown_helper.rb | 2 -- lib/gitlab/markdown.rb | 1 - 2 files changed, 3 deletions(-) diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 1ebfd92f119..9890ec7c757 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -46,7 +46,6 @@ module GitlabMarkdownHelper def markdown(text, context = {}) context.merge!( - current_user: current_user, path: @path, project: @project, project_wiki: @project_wiki, @@ -60,7 +59,6 @@ module GitlabMarkdownHelper # with a custom pipeline depending on the content being rendered def gfm(text, options = {}) options.merge!( - current_user: current_user, path: @path, project: @project, project_wiki: @project_wiki, diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index e07fdb702fc..478851fc656 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -85,7 +85,6 @@ module Gitlab no_header_anchors: options[:no_header_anchors], # ReferenceFilter - current_user: options[:current_user], only_path: options[:reference_only_path], project: options[:project], -- cgit v1.2.1 From 86f706b169bc77b7cf4323dffc5cc7124413eac1 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 1 Sep 2015 18:09:20 -0400 Subject: Fix GitlabMarkdownHelper spec --- spec/helpers/gitlab_markdown_helper_spec.rb | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 5639b3db913..0c2b3003092 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -11,12 +11,15 @@ describe GitlabMarkdownHelper do let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:snippet) { create(:project_snippet, project: project) } - # Helper expects a current_user method. - let(:current_user) { user } - before do + # Ensure the generated reference links aren't redacted + project.team << [user, :master] + # Helper expects a @project instance variable - @project = project + helper.instance_variable_set(:@project, project) + + # Stub the `current_user` helper + allow(helper).to receive(:current_user).and_return(user) end describe "#markdown" do @@ -25,17 +28,17 @@ describe GitlabMarkdownHelper do it "should link to the merge request" do expected = namespace_project_merge_request_path(project.namespace, project, merge_request) - expect(markdown(actual)).to match(expected) + expect(helper.markdown(actual)).to match(expected) end it "should link to the commit" do expected = namespace_project_commit_path(project.namespace, project, commit) - expect(markdown(actual)).to match(expected) + expect(helper.markdown(actual)).to match(expected) end it "should link to the issue" do expected = namespace_project_issue_path(project.namespace, project, issue) - expect(markdown(actual)).to match(expected) + expect(helper.markdown(actual)).to match(expected) end end end @@ -45,7 +48,7 @@ describe GitlabMarkdownHelper do let(:issues) { create_list(:issue, 2, project: project) } it 'should handle references nested in links with all the text' do - actual = link_to_gfm("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real", commit_path) + actual = helper.link_to_gfm("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real", commit_path) doc = Nokogiri::HTML.parse(actual) # Make sure we didn't create invalid markup @@ -75,7 +78,7 @@ describe GitlabMarkdownHelper do end it 'should forward HTML options' do - actual = link_to_gfm("Fixed in #{commit.id}", commit_path, class: 'foo') + actual = helper.link_to_gfm("Fixed in #{commit.id}", commit_path, class: 'foo') doc = Nokogiri::HTML.parse(actual) expect(doc.css('a')).to satisfy do |v| @@ -86,13 +89,13 @@ describe GitlabMarkdownHelper do it "escapes HTML passed in as the body" do actual = "This is a

test

- see #{issues[0].to_reference}" - expect(link_to_gfm(actual, commit_path)). + expect(helper.link_to_gfm(actual, commit_path)). to match('<h1>test</h1>') end it 'ignores reference links when they are the entire body' do text = issues[0].to_reference - act = link_to_gfm(text, '/foo') + act = helper.link_to_gfm(text, '/foo') expect(act).to eq %Q(#{issues[0].to_reference}) end end -- cgit v1.2.1 From ca8d225307280750597150b0b969998f99aad4a5 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 1 Sep 2015 18:09:52 -0400 Subject: Update MarkdownFeature support class - Memoize variables a bit more cleanly - Add user to project's team --- spec/support/markdown_feature.rb | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/spec/support/markdown_feature.rb b/spec/support/markdown_feature.rb index 39a64391460..bedc1a7f1db 100644 --- a/spec/support/markdown_feature.rb +++ b/spec/support/markdown_feature.rb @@ -15,18 +15,17 @@ class MarkdownFeature end def group - unless @group - @group = create(:group) - @group.add_developer(user) + @group ||= create(:group).tap do |group| + group.add_developer(user) end - - @group end # Direct references ---------------------------------------------------------- def project - @project ||= create(:project) + @project ||= create(:project).tap do |project| + project.team << [user, :master] + end end def issue @@ -46,12 +45,10 @@ class MarkdownFeature end def commit_range - unless @commit_range + @commit_range ||= begin commit2 = project.commit('HEAD~3') - @commit_range = CommitRange.new("#{commit.id}...#{commit2.id}", project) + CommitRange.new("#{commit.id}...#{commit2.id}", project) end - - @commit_range end def simple_label @@ -65,13 +62,12 @@ class MarkdownFeature # Cross-references ----------------------------------------------------------- def xproject - unless @xproject + @xproject ||= begin namespace = create(:namespace, name: 'cross-reference') - @xproject = create(:project, namespace: namespace) - @xproject.team << [user, :developer] + create(:project, namespace: namespace) do |project| + project.team << [user, :developer] + end end - - @xproject end def xissue @@ -91,12 +87,10 @@ class MarkdownFeature end def xcommit_range - unless @xcommit_range + @xcommit_range ||= begin xcommit2 = xproject.commit('HEAD~2') - @xcommit_range = CommitRange.new("#{xcommit.id}...#{xcommit2.id}", xproject) + CommitRange.new("#{xcommit.id}...#{xcommit2.id}", xproject) end - - @xcommit_range end def raw_markdown -- cgit v1.2.1 From 5794d65a0743343bfaa367d10d7b0aaa82e20a25 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 1 Sep 2015 18:16:56 -0400 Subject: Add post_process method to Gitlab::Markdown --- app/helpers/gitlab_markdown_helper.rb | 6 ++++-- lib/gitlab/markdown.rb | 22 ++++++++++++++++++++++ spec/features/markdown_spec.rb | 2 +- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 9890ec7c757..f2cab2840d4 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -52,7 +52,8 @@ module GitlabMarkdownHelper ref: @ref ) - Gitlab::Markdown.render(text, context) + html = Gitlab::Markdown.render(text, context) + Gitlab::Markdown.post_process(html, current_user) end # TODO (rspeicher): Remove all usages of this helper and just call `markdown` @@ -65,7 +66,8 @@ module GitlabMarkdownHelper ref: @ref ) - Gitlab::Markdown.gfm(text, options) + html = Gitlab::Markdown.gfm(text, options) + Gitlab::Markdown.post_process(html, current_user) end def asciidoc(text) diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 478851fc656..dbb8da3f0ad 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -31,6 +31,24 @@ module Gitlab renderer.render(markdown) end + # Perform post-processing on an HTML String + # + # This method is used to perform state-dependent changes to a String of + # HTML, such as removing references that the current user doesn't have + # permission to make (`RedactorFilter`). + # + # html - String to process + # for_user - User state + # + # Returns an HTML-safe String + def self.post_process(html, for_user) + result = post_processor.call(html, current_user: for_user) + + result[:output]. + to_html. + html_safe + end + # Provide autoload paths for filters to prevent a circular dependency error autoload :AutolinkFilter, 'gitlab/markdown/autolink_filter' autoload :CommitRangeReferenceFilter, 'gitlab/markdown/commit_range_reference_filter' @@ -115,6 +133,10 @@ module Gitlab end end + def self.post_processor + @post_processor ||= HTML::Pipeline.new([Gitlab::Markdown::RedactorFilter]) + end + def self.redcarpet_options # https://github.com/vmg/redcarpet#and-its-like-really-simple-to-use @redcarpet_options ||= { diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb index c557a1061af..fdd8cf07b12 100644 --- a/spec/features/markdown_spec.rb +++ b/spec/features/markdown_spec.rb @@ -220,7 +220,7 @@ describe 'GitLab Markdown', feature: true do end end - # `markdown` calls these two methods + # Fake a `current_user` helper def current_user @feat.user end -- cgit v1.2.1 From 21cacb36a728dd6e1d1bee77680a1c78645d1765 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 1 Sep 2015 18:32:52 -0400 Subject: Add RedactorFilter specs for invalid Group and Project references --- spec/lib/gitlab/markdown/redactor_filter_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/lib/gitlab/markdown/redactor_filter_spec.rb b/spec/lib/gitlab/markdown/redactor_filter_spec.rb index a6cf3c64236..4ffba9ac7b1 100644 --- a/spec/lib/gitlab/markdown/redactor_filter_spec.rb +++ b/spec/lib/gitlab/markdown/redactor_filter_spec.rb @@ -37,6 +37,12 @@ module Gitlab::Markdown expect(doc.css('a').length).to eq 1 end + + it 'handles invalid Group references' do + link = reference_link(group_id: 12345) + + expect { filter(link) }.not_to raise_error + end end context 'with data-project-id' do @@ -60,6 +66,12 @@ module Gitlab::Markdown expect(doc.css('a').length).to eq 1 end + + it 'handles invalid Project references' do + link = reference_link(project_id: 12345) + + expect { filter(link) }.not_to raise_error + end end context 'with data-user-id' do -- cgit v1.2.1 From e5d89c1084d1c1f29bfabe50718debf8867f760e Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 2 Sep 2015 23:45:42 -0400 Subject: Remove unnecessary current_user context from filter specs --- .../markdown/cross_project_reference_spec.rb | 24 ++++++++-------------- .../gitlab/markdown/user_reference_filter_spec.rb | 11 +++------- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/spec/lib/gitlab/markdown/cross_project_reference_spec.rb b/spec/lib/gitlab/markdown/cross_project_reference_spec.rb index 6490d6f7a42..8d4f9e403a6 100644 --- a/spec/lib/gitlab/markdown/cross_project_reference_spec.rb +++ b/spec/lib/gitlab/markdown/cross_project_reference_spec.rb @@ -2,20 +2,16 @@ require 'spec_helper' module Gitlab::Markdown describe CrossProjectReference do - # context in the html-pipeline sense, not in the rspec sense - let(:context) do - { - current_user: double('user'), - project: double('project') - } - end - include described_class describe '#project_from_ref' do context 'when no project was referenced' do it 'returns the project from context' do - expect(project_from_ref(nil)).to eq context[:project] + project = double + + allow(self).to receive(:context).and_return({ project: project }) + + expect(project_from_ref(nil)).to eq project end end @@ -26,17 +22,13 @@ module Gitlab::Markdown end context 'when referenced project exists' do - let(:project2) { double('referenced project') } + it 'returns the referenced project' do + project2 = double('referenced project') - before do expect(Project).to receive(:find_with_namespace). with('cross/reference').and_return(project2) - end - context 'and the user has permission to read it' do - it 'returns the referenced project' do - expect(project_from_ref('cross/reference')).to eq project2 - end + expect(project_from_ref('cross/reference')).to eq project2 end end end diff --git a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb index d3028cd3b88..c206cf4b745 100644 --- a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb @@ -80,20 +80,15 @@ module Gitlab::Markdown context 'mentioning a group' do let(:group) { create(:group) } - let(:user) { create(:user) } let(:reference) { group.to_reference } - before do - group.add_developer(user) - end - it 'links to the Group' do - doc = filter("Hey #{reference}", current_user: user) + doc = filter("Hey #{reference}") expect(doc.css('a').first.attr('href')).to eq urls.group_url(group) end it 'includes a data-group-id attribute' do - doc = filter("Hey #{reference}", current_user: user) + doc = filter("Hey #{reference}") link = doc.css('a').first expect(link).to have_attribute('data-group-id') @@ -101,7 +96,7 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("Hey #{reference}", current_user: user) + result = pipeline_result("Hey #{reference}") expect(result[:references][:user]).to eq group.users end end -- cgit v1.2.1 From 4bd92e681e0a6d2a8d7e1ef44d9f248394833d09 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 3 Sep 2015 16:38:35 -0400 Subject: Return early from markdown and gfm when text is empty --- app/helpers/gitlab_markdown_helper.rb | 4 ++++ app/views/events/_event_issue.atom.haml | 3 +-- app/views/events/_event_merge_request.atom.haml | 3 +-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index f2cab2840d4..803578f1911 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -45,6 +45,8 @@ module GitlabMarkdownHelper end def markdown(text, context = {}) + return unless text.present? + context.merge!( path: @path, project: @project, @@ -59,6 +61,8 @@ module GitlabMarkdownHelper # TODO (rspeicher): Remove all usages of this helper and just call `markdown` # with a custom pipeline depending on the content being rendered def gfm(text, options = {}) + return unless text.present? + options.merge!( path: @path, project: @project, diff --git a/app/views/events/_event_issue.atom.haml b/app/views/events/_event_issue.atom.haml index 4259f64c191..4e8d70e4e9d 100644 --- a/app/views/events/_event_issue.atom.haml +++ b/app/views/events/_event_issue.atom.haml @@ -1,3 +1,2 @@ %div{xmlns: "http://www.w3.org/1999/xhtml"} - - if issue.description.present? - = markdown(issue.description, xhtml: true, reference_only_path: false, project: issue.project) + = markdown(issue.description, xhtml: true, reference_only_path: false, project: issue.project) diff --git a/app/views/events/_event_merge_request.atom.haml b/app/views/events/_event_merge_request.atom.haml index e8ed13df783..db2b3550c49 100644 --- a/app/views/events/_event_merge_request.atom.haml +++ b/app/views/events/_event_merge_request.atom.haml @@ -1,3 +1,2 @@ %div{xmlns: "http://www.w3.org/1999/xhtml"} - - if merge_request.description.present? - = markdown(merge_request.description, xhtml: true, reference_only_path: false, project: merge_request.project) + = markdown(merge_request.description, xhtml: true, reference_only_path: false, project: merge_request.project) -- cgit v1.2.1 From 3b690891f36975a35923f14388901f4f2a2c3ed9 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 3 Sep 2015 17:35:50 -0400 Subject: Basic support for an Atom-specific rendering pipeline --- app/helpers/gitlab_markdown_helper.rb | 10 ++- app/views/events/_event_issue.atom.haml | 2 +- app/views/events/_event_merge_request.atom.haml | 2 +- app/views/events/_event_note.atom.haml | 2 +- app/views/events/_event_push.atom.haml | 2 +- lib/gitlab/markdown.rb | 111 +++++++++++++----------- 6 files changed, 70 insertions(+), 59 deletions(-) diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 803578f1911..0d175e1ea18 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -47,15 +47,16 @@ module GitlabMarkdownHelper def markdown(text, context = {}) return unless text.present? - context.merge!( + context.reverse_merge!( path: @path, + pipeline: :default, project: @project, project_wiki: @project_wiki, ref: @ref ) html = Gitlab::Markdown.render(text, context) - Gitlab::Markdown.post_process(html, current_user) + Gitlab::Markdown.post_process(html, pipeline: context[:pipeline], user: current_user) end # TODO (rspeicher): Remove all usages of this helper and just call `markdown` @@ -63,15 +64,16 @@ module GitlabMarkdownHelper def gfm(text, options = {}) return unless text.present? - options.merge!( + options.reverse_merge!( path: @path, + pipeline: :default, project: @project, project_wiki: @project_wiki, ref: @ref ) html = Gitlab::Markdown.gfm(text, options) - Gitlab::Markdown.post_process(html, current_user) + Gitlab::Markdown.post_process(html, pipeline: options[:pipeline], user: current_user) end def asciidoc(text) diff --git a/app/views/events/_event_issue.atom.haml b/app/views/events/_event_issue.atom.haml index 4e8d70e4e9d..fad65310021 100644 --- a/app/views/events/_event_issue.atom.haml +++ b/app/views/events/_event_issue.atom.haml @@ -1,2 +1,2 @@ %div{xmlns: "http://www.w3.org/1999/xhtml"} - = markdown(issue.description, xhtml: true, reference_only_path: false, project: issue.project) + = markdown(issue.description, pipeline: :atom, project: issue.project) diff --git a/app/views/events/_event_merge_request.atom.haml b/app/views/events/_event_merge_request.atom.haml index db2b3550c49..19bdc7b9ca5 100644 --- a/app/views/events/_event_merge_request.atom.haml +++ b/app/views/events/_event_merge_request.atom.haml @@ -1,2 +1,2 @@ %div{xmlns: "http://www.w3.org/1999/xhtml"} - = markdown(merge_request.description, xhtml: true, reference_only_path: false, project: merge_request.project) + = markdown(merge_request.description, pipeline: :atom, project: merge_request.project) diff --git a/app/views/events/_event_note.atom.haml b/app/views/events/_event_note.atom.haml index cfbfba50202..b730ebbd5f9 100644 --- a/app/views/events/_event_note.atom.haml +++ b/app/views/events/_event_note.atom.haml @@ -1,2 +1,2 @@ %div{xmlns: "http://www.w3.org/1999/xhtml"} - = markdown(note.note, xhtml: true, reference_only_path: false, project: note.project) + = markdown(note.note, pipeline: :atom, project: note.project) diff --git a/app/views/events/_event_push.atom.haml b/app/views/events/_event_push.atom.haml index 3625cb49d8b..b271b9daff1 100644 --- a/app/views/events/_event_push.atom.haml +++ b/app/views/events/_event_push.atom.haml @@ -6,7 +6,7 @@ %i at = commit[:timestamp].to_time.to_s(:short) - %blockquote= markdown(escape_once(commit[:message]), xhtml: true, reference_only_path: false, project: event.project) + %blockquote= markdown(escape_once(commit[:message]), pipeline: :atom, project: event.project) - if event.commits_count > 15 %p %i diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index dbb8da3f0ad..476c736a11a 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -7,6 +7,14 @@ module Gitlab module Markdown # Convert a Markdown String into an HTML-safe String of HTML # + # Note that while the returned HTML will have been sanitized of dangerous + # HTML, it may post a risk of information leakage if it's not also passed + # through `post_process`. + # + # Also note that the returned String is always HTML, not XHTML. Views + # requiring XHTML, such as Atom feeds, need to call `post_process` on the + # result, providing the appropriate `pipeline` option. + # # markdown - Markdown String # context - Hash of context options passed to our HTML Pipeline # @@ -38,15 +46,19 @@ module Gitlab # permission to make (`RedactorFilter`). # # html - String to process - # for_user - User state + # options - Hash of options to customize output + # :pipeline - Symbol pipeline type + # :user - User object # # Returns an HTML-safe String - def self.post_process(html, for_user) - result = post_processor.call(html, current_user: for_user) - - result[:output]. - to_html. - html_safe + def self.post_process(html, options) + doc = post_processor.to_document(html, current_user: options[:user]) + + if options[:pipeline] == :atom + doc.to_html(save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML) + else + doc.to_html + end.html_safe end # Provide autoload paths for filters to prevent a circular dependency error @@ -68,26 +80,20 @@ module Gitlab autoload :TaskListFilter, 'gitlab/markdown/task_list_filter' autoload :UserReferenceFilter, 'gitlab/markdown/user_reference_filter' - # Public: Parse the provided text with GitLab-Flavored Markdown + # Public: Parse the provided HTML with GitLab-Flavored Markdown # - # text - the source text - # options - A Hash of options used to customize output (default: {}): - # :xhtml - output XHTML instead of HTML - # :reference_only_path - Use relative path for reference links - def self.gfm(text, options = {}) - return text if text.nil? - - # Duplicate the string so we don't alter the original, then call to_str - # to cast it back to a String instead of a SafeBuffer. This is required - # for gsub calls to work as we need them to. - text = text.dup.to_str - - options.reverse_merge!( - xhtml: false, - reference_only_path: true, - project: options[:project], - current_user: options[:current_user] - ) + # html - HTML String + # options - A Hash of options used to customize output (default: {}) + # :no_header_anchors - Disable header anchors in TableOfContentsFilter + # :path - Current path String + # :pipeline - Symbol pipeline type + # :project - Current Project object + # :project_wiki - Current ProjectWiki object + # :ref - Current ref String + # + # Returns an HTML-safe String + def self.gfm(html, options = {}) + return '' unless html.present? @pipeline ||= HTML::Pipeline.new(filters) @@ -96,47 +102,39 @@ module Gitlab pipeline: options[:pipeline], # EmojiFilter - asset_root: Gitlab.config.gitlab.url, asset_host: Gitlab::Application.config.asset_host, - - # TableOfContentsFilter - no_header_anchors: options[:no_header_anchors], + asset_root: Gitlab.config.gitlab.url, # ReferenceFilter - only_path: options[:reference_only_path], - project: options[:project], + only_path: only_path_pipeline?(options[:pipeline]), + project: options[:project], # RelativeLinkFilter + project_wiki: options[:project_wiki], ref: options[:ref], requested_path: options[:path], - project_wiki: options[:project_wiki] - } - - result = @pipeline.call(text, context) - save_options = 0 - if options[:xhtml] - save_options |= Nokogiri::XML::Node::SaveOptions::AS_XHTML - end - - text = result[:output].to_html(save_with: save_options) + # TableOfContentsFilter + no_header_anchors: options[:no_header_anchors] + } - text.html_safe + @pipeline.to_html(html, context).html_safe end private - def self.renderer - @markdown ||= begin - renderer = Redcarpet::Render::HTML.new - Redcarpet::Markdown.new(renderer, redcarpet_options) + # Check if a pipeline enables the `only_path` context option + # + # Returns Boolean + def self.only_path_pipeline?(pipeline) + case pipeline + when :atom, :email + false + else + true end end - def self.post_processor - @post_processor ||= HTML::Pipeline.new([Gitlab::Markdown::RedactorFilter]) - end - def self.redcarpet_options # https://github.com/vmg/redcarpet#and-its-like-really-simple-to-use @redcarpet_options ||= { @@ -151,6 +149,17 @@ module Gitlab }.freeze end + def self.renderer + @markdown ||= begin + renderer = Redcarpet::Render::HTML.new + Redcarpet::Markdown.new(renderer, redcarpet_options) + end + end + + def self.post_processor + @post_processor ||= HTML::Pipeline.new([Gitlab::Markdown::RedactorFilter]) + end + # Filters used in our pipeline # # SanitizationFilter should come first so that all generated reference HTML -- cgit v1.2.1 From 48837850838c8acb0c02ae60b29e18d287865878 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 3 Sep 2015 17:47:27 -0400 Subject: Remove last remaining `reference_only_path` usages --- app/views/notify/_note_message.html.haml | 2 +- app/views/notify/new_issue_email.html.haml | 2 +- app/views/notify/new_merge_request_email.html.haml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/notify/_note_message.html.haml b/app/views/notify/_note_message.html.haml index 3fd4b04ac84..00cb4aa24cc 100644 --- a/app/views/notify/_note_message.html.haml +++ b/app/views/notify/_note_message.html.haml @@ -1,2 +1,2 @@ %div - = markdown(@note.note, reference_only_path: false) + = markdown(@note.note, pipeline: :email) diff --git a/app/views/notify/new_issue_email.html.haml b/app/views/notify/new_issue_email.html.haml index 53a068be52e..d3b799fca23 100644 --- a/app/views/notify/new_issue_email.html.haml +++ b/app/views/notify/new_issue_email.html.haml @@ -1,5 +1,5 @@ -if @issue.description - = markdown(@issue.description, reference_only_path: false) + = markdown(@issue.description, pipeline: :email) - if @issue.assignee_id.present? %p diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml index 5b7dd117c16..90ebdfc3fe2 100644 --- a/app/views/notify/new_merge_request_email.html.haml +++ b/app/views/notify/new_merge_request_email.html.haml @@ -6,4 +6,4 @@ Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name} -if @merge_request.description - = markdown(@merge_request.description, reference_only_path: false) + = markdown(@merge_request.description, pipeline: :email) -- cgit v1.2.1 From c5280434399ee489eebda254b2d246252df68f2b Mon Sep 17 00:00:00 2001 From: Cristian Bica Date: Thu, 1 Oct 2015 17:05:20 +0300 Subject: Allow users to select the Files view as default project view --- app/assets/stylesheets/pages/projects.scss | 84 +++++++++++++++------------- app/controllers/projects_controller.rb | 5 +- app/helpers/preferences_helper.rb | 8 ++- app/models/user.rb | 2 +- app/views/projects/_files.html.haml | 11 ++++ app/views/projects/show.html.haml | 9 +-- app/views/projects/tree/_blob_item.html.haml | 2 +- app/views/projects/tree/_tree_item.html.haml | 2 +- spec/controllers/projects_controller_spec.rb | 25 +++++++++ 9 files changed, 96 insertions(+), 52 deletions(-) create mode 100644 app/views/projects/_files.html.haml diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index 31051785676..7396de88cff 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -1,6 +1,6 @@ .alert_holder { margin: -16px; - + .alert-link { font-weight: normal; } @@ -31,20 +31,20 @@ margin: -$gl-padding; padding: $gl-padding; padding: 44px 0 17px 0; - + .project-identicon-holder { margin-bottom: 16px; - + .avatar, .identicon { margin: 0 auto; float: none; } - + .identicon { @include border-radius(50%); } } - + .project-home-dropdown { margin: 11px 3px 0; } @@ -86,15 +86,15 @@ top: 17px; margin-bottom: 44px; } - + .project-repo-buttons { margin-top: 12px; margin-bottom: 0px; - + .btn { @include bnt-project; @include btn-info; - + .count { display: inline-block; } @@ -105,7 +105,7 @@ .split-one { display: inline-table; margin-right: 12px; - + a { margin: -1px !important; } @@ -129,10 +129,10 @@ &.git-protocols { padding: 0; border: none; - + .input-group-btn:last-child > .btn { @include border-radius-right(0); - + border-left: 1px solid #c6cacf; margin-left: -2px !important; } @@ -141,55 +141,55 @@ } .projects-search-form { - + .input-group .form-control { height: 42px; } } .input-group-btn { - .btn { + .btn { @include bnt-project; @include btn-middle; - + &:hover { outline: none; } - + &:focus { outline: none; } - + &:active { outline: none; } } - + .active { @include box-shadow(inset 0 0 4px rgba(0, 0, 0, 0.12)); - + border: 1px solid #c6cacf !important; background-color: #e4e7ed !important; } - + .btn-green { @include btn-green } - + } .split-repo-buttons { display: inline-table; margin: 0 12px 0 12px; - + .btn{ @include bnt-project; @include btn-info; } - + .dropdown-toggle { margin: -5px; - } + } } #notification-form { @@ -202,7 +202,7 @@ .open > .dropdown-new.btn { @include box-shadow(inset 0 0 4px rgba(0, 0, 0, 0.12)); - + border: 1px solid #c6cacf !important; background-color: #e4e7ed !important; text-transform: uppercase; @@ -214,21 +214,21 @@ .dropdown-menu { @include box-shadow(rgba(76, 86, 103, 0.247059) 0px 0px 1px 0px, rgba(31, 37, 50, 0.317647) 0px 2px 18px 0px); @include border-radius (0px); - + border: none; padding: 16px 0; font-size: 14px; font-weight: 100; - + li a { color: #5f697a; line-height: 30px; - + &:hover { - background-color: #3084bb !important; + background-color: #3084bb !important; } } - + .fa-fw { margin-right: 8px; } @@ -370,7 +370,7 @@ table.table.protected-branches-list tr.no-border { ul.nav-pills { display:inline-block; } - + .nav-pills li { display:inline; } @@ -378,12 +378,12 @@ table.table.protected-branches-list tr.no-border { .nav > li > a { @include btn-info; @include bnt-project; - + background-color: transparent; border: 1px solid #f7f8fa; margin-left: 12px; } - + li { display:inline; } @@ -418,27 +418,27 @@ pre.light-well { .git-empty { margin: 0 7px 0 7px; - + h5 { color: #5c5d5e; } - + .light-well { @include border-radius (2px); - + color: #5b6169; - font-size: 13px; - line-height: 1.6em; + font-size: 13px; + line-height: 1.6em; } } .prepend-top-20 { margin-top: 20px; - + .btn-remove { @include btn-middle; @include btn-remove; - + float: left !important; } } @@ -446,7 +446,7 @@ pre.light-well { /* * Projects list rendered on dashboard and user page */ - + .projects-list { @include basic-list; @@ -507,6 +507,10 @@ pre.light-well { } } +.project-show-files { + padding-top: 20px; +} + .inline-form { display: inline-block; } diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 213c2a7173b..7158d4b49ac 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -89,7 +89,10 @@ class ProjectsController < ApplicationController if current_user @membership = @project.project_member_by_id(current_user.id) end - + @ref = "master" + @id = "master" + @commit = @project.repository.commit(@ref) + @tree = @project.repository.tree(@commit.id) render :show end else diff --git a/app/helpers/preferences_helper.rb b/app/helpers/preferences_helper.rb index 1b1f4162df4..f888c4a829b 100644 --- a/app/helpers/preferences_helper.rb +++ b/app/helpers/preferences_helper.rb @@ -27,7 +27,8 @@ module PreferencesHelper def project_view_choices [ ['Readme (default)', :readme], - ['Activity view', :activity] + ['Activity view', :activity], + ['Files view', :files] ] end @@ -43,4 +44,9 @@ module PreferencesHelper !current_user || current_user.project_view == 'readme' end + + def current_user_default_project_view + (current_user && current_user.project_view) || + 'readme' + end end diff --git a/app/models/user.rb b/app/models/user.rb index 3879f3fd381..f7a1589f5d0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -176,7 +176,7 @@ class User < ActiveRecord::Base # User's Project preference # Note: When adding an option, it MUST go on the end of the array. - enum project_view: [:readme, :activity] + enum project_view: [:readme, :activity, :files] alias_attribute :private_token, :authentication_token diff --git a/app/views/projects/_files.html.haml b/app/views/projects/_files.html.haml new file mode 100644 index 00000000000..2a99708eb43 --- /dev/null +++ b/app/views/projects/_files.html.haml @@ -0,0 +1,11 @@ += render 'projects/last_push' + +.tree-ref-holder + = render 'shared/ref_switcher', destination: 'tree', path: @path + +- if can? current_user, :download_code, @project + .tree-download-holder + = render 'projects/repositories/download_archive', ref: @ref, btn_class: 'btn-group pull-right hidden-xs hidden-sm', split_button: true + +#tree-holder.tree-holder.clearfix + = render "projects/tree/tree", tree: @tree diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index 6a5fc689803..54afb7de15d 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -64,13 +64,8 @@ Archived project! Repository is read-only %section - - if prefer_readme? - .project-show-readme - = render 'projects/readme' - - else - .project-show-activity - = render 'projects/activity' - + %div{class: "project-show-#{current_user_default_project_view}"} + = render current_user_default_project_view - if current_user - access = user_max_access_in_project(current_user, @project) diff --git a/app/views/projects/tree/_blob_item.html.haml b/app/views/projects/tree/_blob_item.html.haml index 02ecbade219..2ddc5d504fa 100644 --- a/app/views/projects/tree/_blob_item.html.haml +++ b/app/views/projects/tree/_blob_item.html.haml @@ -4,5 +4,5 @@ %span.str-truncated = link_to blob_item.name, namespace_project_blob_path(@project.namespace, @project, tree_join(@id || @commit.id, blob_item.name)) %td.tree_time_ago.cgray - = render 'spinner' + = render 'projects/tree/spinner' %td.hidden-xs.tree_commit diff --git a/app/views/projects/tree/_tree_item.html.haml b/app/views/projects/tree/_tree_item.html.haml index e87138bf980..cf65057e704 100644 --- a/app/views/projects/tree/_tree_item.html.haml +++ b/app/views/projects/tree/_tree_item.html.haml @@ -5,5 +5,5 @@ - path = flatten_tree(tree_item) = link_to path, namespace_project_tree_path(@project.namespace, @project, tree_join(@id || @commit.id, path)) %td.tree_time_ago.cgray - = render 'spinner' + = render 'projects/tree/spinner' %td.hidden-xs.tree_commit diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 29233e9fae6..1e018acf42a 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -21,6 +21,31 @@ describe ProjectsController do expect(response.body).to include("content='#{content}'") end end + + context "rendering default project view" do + render_views + + it "shold render the activity view", focus: true do + allow(controller).to receive(:current_user).and_return(user) + allow(user).to receive(:project_view).and_return('activity') + get :show, namespace_id: public_project.namespace.path, id: public_project.path + expect(response).to render_template('_activity') + end + + it "shold render the readme view", focus: true do + allow(controller).to receive(:current_user).and_return(user) + allow(user).to receive(:project_view).and_return('readme') + get :show, namespace_id: public_project.namespace.path, id: public_project.path + expect(response).to render_template('_readme') + end + + it "shold render the files view", focus: true do + allow(controller).to receive(:current_user).and_return(user) + allow(user).to receive(:project_view).and_return('files') + get :show, namespace_id: public_project.namespace.path, id: public_project.path + expect(response).to render_template('_files') + end + end end describe "POST #toggle_star" do -- cgit v1.2.1 From 12626f029dc4ab345f8afa1fd42a3c04723ec55e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 7 Oct 2015 15:17:43 +0200 Subject: Return strings where expected --- app/helpers/gitlab_markdown_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 40161c5a641..6264b7f82ef 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -45,7 +45,7 @@ module GitlabMarkdownHelper end def markdown(text, context = {}) - return unless text.present? + return "" unless text.present? context.reverse_merge!( path: @path, @@ -62,7 +62,7 @@ module GitlabMarkdownHelper # TODO (rspeicher): Remove all usages of this helper and just call `markdown` # with a custom pipeline depending on the content being rendered def gfm(text, options = {}) - return unless text.present? + return "" unless text.present? options.reverse_merge!( path: @path, -- cgit v1.2.1 From f42cfa9e8757161414f88e50d23b1d618bffad1f Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 7 Oct 2015 17:00:48 +0200 Subject: Refactor reference gathering to use a dedicated filter --- app/helpers/gitlab_markdown_helper.rb | 11 ++- .../markdown/commit_range_reference_filter.rb | 16 +++- lib/gitlab/markdown/commit_reference_filter.rb | 24 ++++-- .../markdown/external_issue_reference_filter.rb | 3 +- lib/gitlab/markdown/issue_reference_filter.rb | 11 ++- lib/gitlab/markdown/label_reference_filter.rb | 11 ++- .../markdown/merge_request_reference_filter.rb | 11 ++- lib/gitlab/markdown/redactor_filter.rb | 44 ++--------- lib/gitlab/markdown/reference_filter.rb | 34 ++++----- lib/gitlab/markdown/reference_gatherer_filter.rb | 49 ++++++++++++ lib/gitlab/markdown/snippet_reference_filter.rb | 11 ++- lib/gitlab/markdown/user_reference_filter.rb | 42 +++++++--- lib/gitlab/reference_extractor.rb | 2 +- spec/helpers/gitlab_markdown_helper_spec.rb | 2 +- .../markdown/commit_range_reference_filter_spec.rb | 22 ++++-- .../markdown/commit_reference_filter_spec.rb | 22 ++++-- .../gitlab/markdown/issue_reference_filter_spec.rb | 22 ++++-- .../gitlab/markdown/label_reference_filter_spec.rb | 18 +++-- .../merge_request_reference_filter_spec.rb | 22 ++++-- spec/lib/gitlab/markdown/redactor_filter_spec.rb | 75 +++++++++--------- .../markdown/reference_gatherer_filter_spec.rb | 89 ++++++++++++++++++++++ .../markdown/snippet_reference_filter_spec.rb | 22 ++++-- .../gitlab/markdown/user_reference_filter_spec.rb | 20 ++--- spec/lib/gitlab/reference_extractor_spec.rb | 4 +- spec/support/filter_spec_helper.rb | 9 ++- 25 files changed, 414 insertions(+), 182 deletions(-) create mode 100644 lib/gitlab/markdown/reference_gatherer_filter.rb create mode 100644 spec/lib/gitlab/markdown/reference_gatherer_filter_spec.rb diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 6264b7f82ef..1b8bb46d25e 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -19,7 +19,8 @@ module GitlabMarkdownHelper escape_once(body) end - gfm_body = Gitlab::Markdown.gfm(escaped_body, project: @project, current_user: current_user) + user = current_user if defined?(current_user) + gfm_body = Gitlab::Markdown.gfm(escaped_body, project: @project, current_user: user) fragment = Nokogiri::HTML::DocumentFragment.parse(gfm_body) if fragment.children.size == 1 && fragment.children[0].name == 'a' @@ -55,8 +56,10 @@ module GitlabMarkdownHelper ref: @ref ) + user = current_user if defined?(current_user) + html = Gitlab::Markdown.render(text, context) - Gitlab::Markdown.post_process(html, pipeline: context[:pipeline], user: current_user) + Gitlab::Markdown.post_process(html, pipeline: context[:pipeline], user: user) end # TODO (rspeicher): Remove all usages of this helper and just call `markdown` @@ -72,8 +75,10 @@ module GitlabMarkdownHelper ref: @ref ) + user = current_user if defined?(current_user) + html = Gitlab::Markdown.gfm(text, options) - Gitlab::Markdown.post_process(html, pipeline: options[:pipeline], user: current_user) + Gitlab::Markdown.post_process(html, pipeline: options[:pipeline], user: user) end def asciidoc(text) diff --git a/lib/gitlab/markdown/commit_range_reference_filter.rb b/lib/gitlab/markdown/commit_range_reference_filter.rb index bb496135d92..e070edae0a4 100644 --- a/lib/gitlab/markdown/commit_range_reference_filter.rb +++ b/lib/gitlab/markdown/commit_range_reference_filter.rb @@ -26,6 +26,18 @@ module Gitlab end end + def self.referenced_by(node) + project = Project.find(node.attr("data-project")) rescue nil + return unless project + + id = node.attr("data-commit-range") + range = CommitRange.new(id, project) + + return unless range.valid_commits? + + { commit_range: range } + end + def initialize(*args) super @@ -53,13 +65,11 @@ module Gitlab range = CommitRange.new(id, project) if range.valid_commits? - push_result(:commit_range, range) - url = url_for_commit_range(project, range) title = range.reference_title klass = reference_class(:commit_range) - data = data_attribute(project.id) + data = data_attribute(project: project.id, commit_range: id) project_ref += '@' if project_ref diff --git a/lib/gitlab/markdown/commit_reference_filter.rb b/lib/gitlab/markdown/commit_reference_filter.rb index fcbb2e936a5..8cdbeb1f9cf 100644 --- a/lib/gitlab/markdown/commit_reference_filter.rb +++ b/lib/gitlab/markdown/commit_reference_filter.rb @@ -26,6 +26,18 @@ module Gitlab end end + def self.referenced_by(node) + project = Project.find(node.attr("data-project")) rescue nil + return unless project + + id = node.attr("data-commit") + commit = commit_from_ref(project, id) + + return unless commit + + { commit: commit } + end + def call replace_text_nodes_matching(Commit.reference_pattern) do |content| commit_link_filter(content) @@ -39,17 +51,15 @@ module Gitlab # Returns a String with commit references replaced with links. All links # have `gfm` and `gfm-commit` class names attached for styling. def commit_link_filter(text) - self.class.references_in(text) do |match, commit_ref, project_ref| + self.class.references_in(text) do |match, id, project_ref| project = self.project_from_ref(project_ref) - if commit = commit_from_ref(project, commit_ref) - push_result(:commit, commit) - + if commit = self.class.commit_from_ref(project, id) url = url_for_commit(project, commit) title = escape_once(commit.link_title) klass = reference_class(:commit) - data = data_attribute(project.id) + data = data_attribute(project: project.id, commit: id) project_ref += '@' if project_ref @@ -62,9 +72,9 @@ module Gitlab end end - def commit_from_ref(project, commit_ref) + def self.commit_from_ref(project, id) if project && project.valid_repo? - project.commit(commit_ref) + project.commit(id) end end diff --git a/lib/gitlab/markdown/external_issue_reference_filter.rb b/lib/gitlab/markdown/external_issue_reference_filter.rb index f7c43e1ca89..8f86f13976a 100644 --- a/lib/gitlab/markdown/external_issue_reference_filter.rb +++ b/lib/gitlab/markdown/external_issue_reference_filter.rb @@ -47,8 +47,9 @@ module Gitlab title = escape_once("Issue in #{project.external_issue_tracker.title}") klass = reference_class(:issue) + data = data_attribute(project: project.id) - %(#{match}) end diff --git a/lib/gitlab/markdown/issue_reference_filter.rb b/lib/gitlab/markdown/issue_reference_filter.rb index 01320f80796..cd2a9b75680 100644 --- a/lib/gitlab/markdown/issue_reference_filter.rb +++ b/lib/gitlab/markdown/issue_reference_filter.rb @@ -27,6 +27,13 @@ module Gitlab end end + def self.referenced_by(node) + issue = Issue.find(node.attr("data-issue")) rescue nil + return unless issue + + { issue: issue } + end + def call replace_text_nodes_matching(Issue.reference_pattern) do |content| issue_link_filter(content) @@ -45,13 +52,11 @@ module Gitlab project = self.project_from_ref(project_ref) if project && issue = project.get_issue(id) - push_result(:issue, issue) - url = url_for_issue(id, project, only_path: context[:only_path]) title = escape_once("Issue: #{issue.title}") klass = reference_class(:issue) - data = data_attribute(project.id) + data = data_attribute(project: project.id, issue: issue.id) %(#{render_colored_label(label)}) diff --git a/lib/gitlab/markdown/merge_request_reference_filter.rb b/lib/gitlab/markdown/merge_request_reference_filter.rb index ecbd263d0e0..440574e574b 100644 --- a/lib/gitlab/markdown/merge_request_reference_filter.rb +++ b/lib/gitlab/markdown/merge_request_reference_filter.rb @@ -27,6 +27,13 @@ module Gitlab end end + def self.referenced_by(node) + merge_request = MergeRequest.find(node.attr("data-merge-request")) rescue nil + return unless merge_request + + { merge_request: merge_request } + end + def call replace_text_nodes_matching(MergeRequest.reference_pattern) do |content| merge_request_link_filter(content) @@ -45,11 +52,9 @@ module Gitlab project = self.project_from_ref(project_ref) if project && merge_request = project.merge_requests.find_by(iid: id) - push_result(:merge_request, merge_request) - title = escape_once("Merge Request: #{merge_request.title}") klass = reference_class(:merge_request) - data = data_attribute(project.id) + data = data_attribute(project: project.id, merge_request: merge_request.id) url = url_for_merge_request(merge_request, project) diff --git a/lib/gitlab/markdown/redactor_filter.rb b/lib/gitlab/markdown/redactor_filter.rb index ae1c3c365bd..07ea6207d22 100644 --- a/lib/gitlab/markdown/redactor_filter.rb +++ b/lib/gitlab/markdown/redactor_filter.rb @@ -19,49 +19,19 @@ module Gitlab doc end + private + def user_can_reference?(node) - if node.has_attribute?('data-group-id') - user_can_reference_group?(node.attr('data-group-id')) - elsif node.has_attribute?('data-project-id') - user_can_reference_project?(node.attr('data-project-id')) - elsif node.has_attribute?('data-user-id') - user_can_reference_user?(node.attr('data-user-id')) + if node.has_attribute?('data-reference-filter') + reference_type = node.attr('data-reference-filter') + reference_filter = reference_type.constantize + + reference_filter.user_can_reference?(current_user, node) else true end end - def user_can_reference_group?(id) - group = Group.find(id) - - group && can?(:read_group, group) - rescue ActiveRecord::RecordNotFound - false - end - - def user_can_reference_project?(id) - project = Project.find(id) - - project && can?(:read_project, project) - rescue ActiveRecord::RecordNotFound - false - end - - def user_can_reference_user?(id) - # Permit all user reference links - true - end - - private - - def abilities - Ability.abilities - end - - def can?(ability, object) - abilities.allowed?(current_user, ability, object) - end - def current_user context[:current_user] end diff --git a/lib/gitlab/markdown/reference_filter.rb b/lib/gitlab/markdown/reference_filter.rb index 9b293c957d6..ea6136b3303 100644 --- a/lib/gitlab/markdown/reference_filter.rb +++ b/lib/gitlab/markdown/reference_filter.rb @@ -15,10 +15,17 @@ module Gitlab # Results: # :references - A Hash of references that were found and replaced. class ReferenceFilter < HTML::Pipeline::Filter - def initialize(*args) - super + def self.user_can_reference?(user, node) + if node.has_attribute?('data-project') + project = Project.find(node.attr('data-project')) rescue nil + Ability.abilities.allowed?(user, :read_project, project) + else + true + end + end - result[:references] = Hash.new { |hash, type| hash[type] = [] } + def self.referenced_by(node) + nil end # Returns a data attribute String to attach to a reference link @@ -28,13 +35,14 @@ module Gitlab # # Examples: # - # data_attribute(1) # => "data-project-id=\"1\"" - # data_attribute(2, :user) # => "data-user-id=\"2\"" - # data_attribute(3, :group) # => "data-group-id=\"3\"" + # data_attribute(project: 1) # => "data-reference-filter=\"SomeReferenceFilter\" data-project=\"1\"" + # data_attribute(user: 2) # => "data-reference-filter=\"SomeReferenceFilter\" data-user=\"2\"" + # data_attribute(group: 3) # => "data-reference-filter=\"SomeReferenceFilter\" data-group=\"3\"" # # Returns a String - def data_attribute(id, type = :project) - %Q(data-#{type}-id="#{id}") + def data_attribute(attributes = {}) + attributes[:reference_filter] = self.class.name + attributes.map { |key, value| %Q(data-#{key.to_s.dasherize}="#{value}") }.join(" ") end def escape_once(html) @@ -59,16 +67,6 @@ module Gitlab context[:project] end - # Add a reference to the pipeline's result Hash - # - # type - Singular Symbol reference type (e.g., :issue, :user, etc.) - # values - One or more Objects to add - def push_result(type, *values) - return if values.empty? - - result[:references][type].push(*values) - end - def reference_class(type) "gfm gfm-#{type}" end diff --git a/lib/gitlab/markdown/reference_gatherer_filter.rb b/lib/gitlab/markdown/reference_gatherer_filter.rb new file mode 100644 index 00000000000..d64671e9550 --- /dev/null +++ b/lib/gitlab/markdown/reference_gatherer_filter.rb @@ -0,0 +1,49 @@ +require 'gitlab/markdown' +require 'html/pipeline/filter' + +module Gitlab + module Markdown + # HTML filter that removes references to records that the current user does + # not have permission to view. + # + # Expected to be run in its own post-processing pipeline. + # + class ReferenceGathererFilter < HTML::Pipeline::Filter + def initialize(*) + super + + result[:references] ||= Hash.new { |hash, type| hash[type] = [] } + end + + def call + doc.css('a.gfm').each do |node| + gather_references(node) + end + + doc + end + + private + + def gather_references(node) + return unless node.has_attribute?('data-reference-filter') + + reference_type = node.attr('data-reference-filter') + reference_filter = reference_type.constantize + + return unless reference_filter.user_can_reference?(current_user, node) + + references = reference_filter.referenced_by(node) + return unless references + + references.each do |type, values| + result[:references][type].push(*values) + end + end + + def current_user + context[:current_user] + end + end + end +end diff --git a/lib/gitlab/markdown/snippet_reference_filter.rb b/lib/gitlab/markdown/snippet_reference_filter.rb index e2cf89cb1d8..a7396e96529 100644 --- a/lib/gitlab/markdown/snippet_reference_filter.rb +++ b/lib/gitlab/markdown/snippet_reference_filter.rb @@ -27,6 +27,13 @@ module Gitlab end end + def self.referenced_by(node) + snippet = Snippet.find(node.attr("data-snippet")) rescue nil + return unless snippet + + { snippet: snippet } + end + def call replace_text_nodes_matching(Snippet.reference_pattern) do |content| snippet_link_filter(content) @@ -45,11 +52,9 @@ module Gitlab project = self.project_from_ref(project_ref) if project && snippet = project.snippets.find_by(id: id) - push_result(:snippet, snippet) - title = escape_once("Snippet: #{snippet.title}") klass = reference_class(:snippet) - data = data_attribute(project.id) + data = data_attribute(project: project.id, snippet: snippet.id) url = url_for_snippet(snippet, project) diff --git a/lib/gitlab/markdown/user_reference_filter.rb b/lib/gitlab/markdown/user_reference_filter.rb index c08811effb2..0d2be7499b7 100644 --- a/lib/gitlab/markdown/user_reference_filter.rb +++ b/lib/gitlab/markdown/user_reference_filter.rb @@ -23,6 +23,34 @@ module Gitlab end end + def self.referenced_by(node) + if node.has_attribute?('data-group') + group = Group.find(node.attr('data-group')) rescue nil + return unless group + + { user: group.users } + elsif node.has_attribute?('data-user') + user = User.find(node.attr('data-user')) rescue nil + return unless user + + { user: user } + elsif node.has_attribute?('data-project') + project = Project.find(node.attr('data-project')) rescue nil + return unless project + + { user: project.team.members.flatten } + end + end + + def self.user_can_reference?(user, node) + if node.has_attribute?('data-group') + group = Group.find(node.attr('data-group')) rescue nil + Ability.abilities.allowed?(user, :read_group, group) + else + super + end + end + def call replace_text_nodes_matching(User.reference_pattern) do |content| user_link_filter(content) @@ -61,14 +89,12 @@ module Gitlab def link_to_all project = context[:project] - # FIXME (rspeicher): Law of Demeter - push_result(:user, *project.team.members.flatten) - url = urls.namespace_project_url(project.namespace, project, only_path: context[:only_path]) + data = data_attribute(project: project.id) text = User.reference_prefix + 'all' - %(#{text}) + %(#{text}) end def link_to_namespace(namespace) @@ -80,20 +106,16 @@ module Gitlab end def link_to_group(group, namespace) - push_result(:user, *namespace.users) - url = urls.group_url(group, only_path: context[:only_path]) - data = data_attribute(namespace.id, :group) + data = data_attribute(group: namespace.id) text = Group.reference_prefix + group %(#{text}) end def link_to_user(user, namespace) - push_result(:user, namespace.owner) - url = urls.user_url(user, only_path: context[:only_path]) - data = data_attribute(namespace.owner_id, :user) + data = data_attribute(user: namespace.owner_id) text = User.reference_prefix + user %(#{text}) diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 0961bd80421..2e546ef0d54 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -50,7 +50,7 @@ module Gitlab ignore_blockquotes: true } - pipeline = HTML::Pipeline.new([filter], context) + pipeline = HTML::Pipeline.new([filter, Gitlab::Markdown::ReferenceGathererFilter], context) result = pipeline.call(@text) result[:references][filter_type] diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 5d471f2f6ee..762ec25c4f5 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -44,7 +44,7 @@ describe GitlabMarkdownHelper do describe "override default project" do let(:actual) { issue.to_reference } - let(:second_project) { create(:project) } + let(:second_project) { create(:project, :public) } let(:second_issue) { create(:issue, project: second_project) } it 'should link to the issue' do diff --git a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb index 6813d6db14c..e5b8d723fe5 100644 --- a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb @@ -4,7 +4,7 @@ module Gitlab::Markdown describe CommitRangeReferenceFilter do include FilterSpecHelper - let(:project) { create(:project) } + let(:project) { create(:project, :public) } let(:commit1) { project.commit } let(:commit2) { project.commit("HEAD~2") } @@ -75,12 +75,20 @@ module Gitlab::Markdown expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-commit_range' end - it 'includes a data-project-id attribute' do + it 'includes a data-project attribute' do doc = filter("See #{reference}") link = doc.css('a').first - expect(link).to have_attribute('data-project-id') - expect(link.attr('data-project-id')).to eq project.id.to_s + expect(link).to have_attribute('data-project') + expect(link.attr('data-project')).to eq project.id.to_s + end + + it 'includes a data-commit-range attribute' do + doc = filter("See #{reference}") + link = doc.css('a').first + + expect(link).to have_attribute('data-commit-range') + expect(link.attr('data-commit-range')).to eq range.to_reference end it 'supports an :only_path option' do @@ -92,14 +100,14 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("See #{reference}") + result = reference_pipeline_result("See #{reference}") expect(result[:references][:commit_range]).not_to be_empty end end context 'cross-project reference' do let(:namespace) { create(:namespace, name: 'cross-reference') } - let(:project2) { create(:project, namespace: namespace) } + let(:project2) { create(:project, :public, namespace: namespace) } let(:reference) { range.to_reference(project) } before do @@ -129,7 +137,7 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("See #{reference}") + result = reference_pipeline_result("See #{reference}") expect(result[:references][:commit_range]).not_to be_empty end end diff --git a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb index f937b4f50ee..d080efbf3d4 100644 --- a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb @@ -4,7 +4,7 @@ module Gitlab::Markdown describe CommitReferenceFilter do include FilterSpecHelper - let(:project) { create(:project) } + let(:project) { create(:project, :public) } let(:commit) { project.commit } it 'requires project context' do @@ -71,12 +71,20 @@ module Gitlab::Markdown expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-commit' end - it 'includes a data-project-id attribute' do + it 'includes a data-project attribute' do doc = filter("See #{reference}") link = doc.css('a').first - expect(link).to have_attribute('data-project-id') - expect(link.attr('data-project-id')).to eq project.id.to_s + expect(link).to have_attribute('data-project') + expect(link.attr('data-project')).to eq project.id.to_s + end + + it 'includes a data-commit attribute' do + doc = filter("See #{reference}") + link = doc.css('a').first + + expect(link).to have_attribute('data-commit') + expect(link.attr('data-commit')).to eq commit.id end it 'supports an :only_path context' do @@ -88,14 +96,14 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("See #{reference}") + result = reference_pipeline_result("See #{reference}") expect(result[:references][:commit]).not_to be_empty end end context 'cross-project reference' do let(:namespace) { create(:namespace, name: 'cross-reference') } - let(:project2) { create(:project, namespace: namespace) } + let(:project2) { create(:project, :public, namespace: namespace) } let(:commit) { project2.commit } let(:reference) { commit.to_reference(project) } @@ -119,7 +127,7 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("See #{reference}") + result = reference_pipeline_result("See #{reference}") expect(result[:references][:commit]).not_to be_empty end end diff --git a/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb b/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb index 96787954516..94c80ae6611 100644 --- a/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb @@ -8,7 +8,7 @@ module Gitlab::Markdown IssuesHelper end - let(:project) { create(:empty_project) } + let(:project) { create(:empty_project, :public) } let(:issue) { create(:issue, project: project) } it 'requires project context' do @@ -68,12 +68,20 @@ module Gitlab::Markdown expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-issue' end - it 'includes a data-project-id attribute' do + it 'includes a data-project attribute' do doc = filter("Issue #{reference}") link = doc.css('a').first - expect(link).to have_attribute('data-project-id') - expect(link.attr('data-project-id')).to eq project.id.to_s + expect(link).to have_attribute('data-project') + expect(link.attr('data-project')).to eq project.id.to_s + end + + it 'includes a data-issue attribute' do + doc = filter("See #{reference}") + link = doc.css('a').first + + expect(link).to have_attribute('data-issue') + expect(link.attr('data-issue')).to eq issue.id.to_s end it 'supports an :only_path context' do @@ -85,14 +93,14 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("Fixed #{reference}") + result = reference_pipeline_result("Fixed #{reference}") expect(result[:references][:issue]).to eq [issue] end end context 'cross-project reference' do let(:namespace) { create(:namespace, name: 'cross-reference') } - let(:project2) { create(:empty_project, namespace: namespace) } + let(:project2) { create(:empty_project, :public, namespace: namespace) } let(:issue) { create(:issue, project: project2) } let(:reference) { issue.to_reference(project) } @@ -123,7 +131,7 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("Fixed #{reference}") + result = reference_pipeline_result("Fixed #{reference}") expect(result[:references][:issue]).to eq [issue] end end diff --git a/spec/lib/gitlab/markdown/label_reference_filter_spec.rb b/spec/lib/gitlab/markdown/label_reference_filter_spec.rb index e32089de376..fc21b65a843 100644 --- a/spec/lib/gitlab/markdown/label_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/label_reference_filter_spec.rb @@ -5,7 +5,7 @@ module Gitlab::Markdown describe LabelReferenceFilter do include FilterSpecHelper - let(:project) { create(:empty_project) } + let(:project) { create(:empty_project, :public) } let(:label) { create(:label, project: project) } let(:reference) { label.to_reference } @@ -25,12 +25,20 @@ module Gitlab::Markdown expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-label' end - it 'includes a data-project-id attribute' do + it 'includes a data-project attribute' do doc = filter("Label #{reference}") link = doc.css('a').first - expect(link).to have_attribute('data-project-id') - expect(link.attr('data-project-id')).to eq project.id.to_s + expect(link).to have_attribute('data-project') + expect(link.attr('data-project')).to eq project.id.to_s + end + + it 'includes a data-label attribute' do + doc = filter("See #{reference}") + link = doc.css('a').first + + expect(link).to have_attribute('data-label') + expect(link.attr('data-label')).to eq label.id.to_s end it 'supports an :only_path context' do @@ -42,7 +50,7 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("Label #{reference}") + result = reference_pipeline_result("Label #{reference}") expect(result[:references][:label]).to eq [label] end diff --git a/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb b/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb index feba08f7200..3ef6cdfff33 100644 --- a/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb @@ -4,7 +4,7 @@ module Gitlab::Markdown describe MergeRequestReferenceFilter do include FilterSpecHelper - let(:project) { create(:project) } + let(:project) { create(:project, :public) } let(:merge) { create(:merge_request, source_project: project) } it 'requires project context' do @@ -56,12 +56,20 @@ module Gitlab::Markdown expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-merge_request' end - it 'includes a data-project-id attribute' do + it 'includes a data-project attribute' do doc = filter("Merge #{reference}") link = doc.css('a').first - expect(link).to have_attribute('data-project-id') - expect(link.attr('data-project-id')).to eq project.id.to_s + expect(link).to have_attribute('data-project') + expect(link.attr('data-project')).to eq project.id.to_s + end + + it 'includes a data-merge-request attribute' do + doc = filter("See #{reference}") + link = doc.css('a').first + + expect(link).to have_attribute('data-merge-request') + expect(link.attr('data-merge-request')).to eq merge.id.to_s end it 'supports an :only_path context' do @@ -73,14 +81,14 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("Merge #{reference}") + result = reference_pipeline_result("Merge #{reference}") expect(result[:references][:merge_request]).to eq [merge] end end context 'cross-project reference' do let(:namespace) { create(:namespace, name: 'cross-reference') } - let(:project2) { create(:project, namespace: namespace) } + let(:project2) { create(:project, :public, namespace: namespace) } let(:merge) { create(:merge_request, source_project: project2) } let(:reference) { merge.to_reference(project) } @@ -104,7 +112,7 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("Merge #{reference}") + result = reference_pipeline_result("Merge #{reference}") expect(result[:references][:merge_request]).to eq [merge] end end diff --git a/spec/lib/gitlab/markdown/redactor_filter_spec.rb b/spec/lib/gitlab/markdown/redactor_filter_spec.rb index 4ffba9ac7b1..eea3f1cf370 100644 --- a/spec/lib/gitlab/markdown/redactor_filter_spec.rb +++ b/spec/lib/gitlab/markdown/redactor_filter_spec.rb @@ -16,72 +16,75 @@ module Gitlab::Markdown link_to('text', '', class: 'gfm', data: data) end - context 'with data-group-id' do - it 'removes unpermitted Group references' do + context 'with data-project' do + it 'removes unpermitted Project references' do user = create(:user) - group = create(:group) + project = create(:empty_project) - link = reference_link(group_id: group.id) + link = reference_link(project: project.id, reference_filter: Gitlab::Markdown::ReferenceFilter.name) doc = filter(link, current_user: user) expect(doc.css('a').length).to eq 0 end - it 'allows permitted Group references' do + it 'allows permitted Project references' do user = create(:user) - group = create(:group) - group.add_developer(user) + project = create(:empty_project) + project.team << [user, :master] - link = reference_link(group_id: group.id) + link = reference_link(project: project.id, reference_filter: Gitlab::Markdown::ReferenceFilter.name) doc = filter(link, current_user: user) expect(doc.css('a').length).to eq 1 end - it 'handles invalid Group references' do - link = reference_link(group_id: 12345) + it 'handles invalid Project references' do + link = reference_link(project: 12345, reference_filter: Gitlab::Markdown::ReferenceFilter.name) expect { filter(link) }.not_to raise_error end end - context 'with data-project-id' do - it 'removes unpermitted Project references' do - user = create(:user) - project = create(:empty_project) + context "for user references" do - link = reference_link(project_id: project.id) - doc = filter(link, current_user: user) + context 'with data-group' do + it 'removes unpermitted Group references' do + user = create(:user) + group = create(:group) - expect(doc.css('a').length).to eq 0 - end + link = reference_link(group: group.id, reference_filter: Gitlab::Markdown::UserReferenceFilter.name) + doc = filter(link, current_user: user) - it 'allows permitted Project references' do - user = create(:user) - project = create(:empty_project) - project.team << [user, :master] + expect(doc.css('a').length).to eq 0 + end - link = reference_link(project_id: project.id) - doc = filter(link, current_user: user) + it 'allows permitted Group references' do + user = create(:user) + group = create(:group) + group.add_developer(user) - expect(doc.css('a').length).to eq 1 - end + link = reference_link(group: group.id, reference_filter: Gitlab::Markdown::UserReferenceFilter.name) + doc = filter(link, current_user: user) - it 'handles invalid Project references' do - link = reference_link(project_id: 12345) + expect(doc.css('a').length).to eq 1 + end - expect { filter(link) }.not_to raise_error + it 'handles invalid Group references' do + link = reference_link(group: 12345, reference_filter: Gitlab::Markdown::UserReferenceFilter.name) + + expect { filter(link) }.not_to raise_error + end end - end - context 'with data-user-id' do - it 'allows any User reference' do - user = create(:user) + context 'with data-user' do + it 'allows any User reference' do + user = create(:user) - link = reference_link(user_id: user.id) - doc = filter(link) + link = reference_link(user: user.id, reference_filter: Gitlab::Markdown::UserReferenceFilter.name) + doc = filter(link) - expect(doc.css('a').length).to eq 1 + expect(doc.css('a').length).to eq 1 + end end end end diff --git a/spec/lib/gitlab/markdown/reference_gatherer_filter_spec.rb b/spec/lib/gitlab/markdown/reference_gatherer_filter_spec.rb new file mode 100644 index 00000000000..4fa473ad191 --- /dev/null +++ b/spec/lib/gitlab/markdown/reference_gatherer_filter_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +module Gitlab::Markdown + describe ReferenceGathererFilter do + include ActionView::Helpers::UrlHelper + include FilterSpecHelper + + def reference_link(data) + link_to('text', '', class: 'gfm', data: data) + end + + context "for issue references" do + + context 'with data-project' do + it 'removes unpermitted Project references' do + user = create(:user) + project = create(:empty_project) + issue = create(:issue, project: project) + + link = reference_link(project: project.id, issue: issue.id, reference_filter: Gitlab::Markdown::IssueReferenceFilter.name) + result = pipeline_result(link, current_user: user) + + expect(result[:references][:issue]).to be_empty + end + + it 'allows permitted Project references' do + user = create(:user) + project = create(:empty_project) + issue = create(:issue, project: project) + project.team << [user, :master] + + link = reference_link(project: project.id, issue: issue.id, reference_filter: Gitlab::Markdown::IssueReferenceFilter.name) + result = pipeline_result(link, current_user: user) + + expect(result[:references][:issue]).to eq([issue]) + end + + it 'handles invalid Project references' do + link = reference_link(project: 12345, issue: 12345, reference_filter: Gitlab::Markdown::IssueReferenceFilter.name) + + expect { pipeline_result(link) }.not_to raise_error + end + end + end + + context "for user references" do + + context 'with data-group' do + it 'removes unpermitted Group references' do + user = create(:user) + group = create(:group) + + link = reference_link(group: group.id, reference_filter: Gitlab::Markdown::UserReferenceFilter.name) + result = pipeline_result(link, current_user: user) + + expect(result[:references][:user]).to be_empty + end + + it 'allows permitted Group references' do + user = create(:user) + group = create(:group) + group.add_developer(user) + + link = reference_link(group: group.id, reference_filter: Gitlab::Markdown::UserReferenceFilter.name) + result = pipeline_result(link, current_user: user) + + expect(result[:references][:user]).to eq([user]) + end + + it 'handles invalid Group references' do + link = reference_link(group: 12345, reference_filter: Gitlab::Markdown::UserReferenceFilter.name) + + expect { pipeline_result(link) }.not_to raise_error + end + end + + context 'with data-user' do + it 'allows any User reference' do + user = create(:user) + + link = reference_link(user: user.id, reference_filter: Gitlab::Markdown::UserReferenceFilter.name) + result = pipeline_result(link) + + expect(result[:references][:user]).to eq([user]) + end + end + end + end +end diff --git a/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb b/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb index 02d581a7c46..9d9652dba46 100644 --- a/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb @@ -4,7 +4,7 @@ module Gitlab::Markdown describe SnippetReferenceFilter do include FilterSpecHelper - let(:project) { create(:empty_project) } + let(:project) { create(:empty_project, :public) } let(:snippet) { create(:project_snippet, project: project) } let(:reference) { snippet.to_reference } @@ -55,12 +55,20 @@ module Gitlab::Markdown expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-snippet' end - it 'includes a data-project-id attribute' do + it 'includes a data-project attribute' do doc = filter("Snippet #{reference}") link = doc.css('a').first - expect(link).to have_attribute('data-project-id') - expect(link.attr('data-project-id')).to eq project.id.to_s + expect(link).to have_attribute('data-project') + expect(link.attr('data-project')).to eq project.id.to_s + end + + it 'includes a data-snippet attribute' do + doc = filter("See #{reference}") + link = doc.css('a').first + + expect(link).to have_attribute('data-snippet') + expect(link.attr('data-snippet')).to eq snippet.id.to_s end it 'supports an :only_path context' do @@ -72,14 +80,14 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("Snippet #{reference}") + result = reference_pipeline_result("Snippet #{reference}") expect(result[:references][:snippet]).to eq [snippet] end end context 'cross-project reference' do let(:namespace) { create(:namespace, name: 'cross-reference') } - let(:project2) { create(:empty_project, namespace: namespace) } + let(:project2) { create(:empty_project, :public, namespace: namespace) } let(:snippet) { create(:project_snippet, project: project2) } let(:reference) { snippet.to_reference(project) } @@ -102,7 +110,7 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("Snippet #{reference}") + result = reference_pipeline_result("Snippet #{reference}") expect(result[:references][:snippet]).to eq [snippet] end end diff --git a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb index c206cf4b745..d9e0d7c42db 100644 --- a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb @@ -4,7 +4,7 @@ module Gitlab::Markdown describe UserReferenceFilter do include FilterSpecHelper - let(:project) { create(:empty_project) } + let(:project) { create(:empty_project, :public) } let(:user) { create(:user) } let(:reference) { user.to_reference } @@ -39,7 +39,7 @@ module Gitlab::Markdown end it 'adds to the results hash' do - result = pipeline_result("Hey #{reference}") + result = reference_pipeline_result("Hey #{reference}") expect(result[:references][:user]).to eq [project.creator] end end @@ -64,16 +64,16 @@ module Gitlab::Markdown expect(doc.css('a').length).to eq 1 end - it 'includes a data-user-id attribute' do + it 'includes a data-user attribute' do doc = filter("Hey #{reference}") link = doc.css('a').first - expect(link).to have_attribute('data-user-id') - expect(link.attr('data-user-id')).to eq user.namespace.owner_id.to_s + expect(link).to have_attribute('data-user') + expect(link.attr('data-user')).to eq user.namespace.owner_id.to_s end it 'adds to the results hash' do - result = pipeline_result("Hey #{reference}") + result = reference_pipeline_result("Hey #{reference}") expect(result[:references][:user]).to eq [user] end end @@ -87,16 +87,16 @@ module Gitlab::Markdown expect(doc.css('a').first.attr('href')).to eq urls.group_url(group) end - it 'includes a data-group-id attribute' do + it 'includes a data-group attribute' do doc = filter("Hey #{reference}") link = doc.css('a').first - expect(link).to have_attribute('data-group-id') - expect(link.attr('data-group-id')).to eq group.id.to_s + expect(link).to have_attribute('data-group') + expect(link.attr('data-group')).to eq group.id.to_s end it 'adds to the results hash' do - result = pipeline_result("Hey #{reference}") + result = reference_pipeline_result("Hey #{reference}") expect(result[:references][:user]).to eq group.users end end diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index 088e34f050c..6d7a067e4e0 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/spec/lib/gitlab/reference_extractor_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::ReferenceExtractor do let(:project) { create(:project) } - subject { Gitlab::ReferenceExtractor.new(project, project.creator) } + subject { Gitlab::ReferenceExtractor.new(project, project.owner) } it 'accesses valid user objects' do @u_foo = create(:user, username: 'foo') @@ -102,7 +102,7 @@ describe Gitlab::ReferenceExtractor do let(:issue) { create(:issue, project: other_project) } before do - other_project.team << [project.creator, :developer] + other_project.team << [project.owner, :developer] end it 'handles project issue references' do diff --git a/spec/support/filter_spec_helper.rb b/spec/support/filter_spec_helper.rb index 56ce88abb48..97e5c270a59 100644 --- a/spec/support/filter_spec_helper.rb +++ b/spec/support/filter_spec_helper.rb @@ -29,12 +29,19 @@ module FilterSpecHelper # # Returns the Hash def pipeline_result(body, contexts = {}) - contexts.reverse_merge!(project: project) + contexts.reverse_merge!(project: project) if defined?(project) pipeline = HTML::Pipeline.new([described_class], contexts) pipeline.call(body) end + def reference_pipeline_result(body, contexts = {}) + contexts.reverse_merge!(project: project) if defined?(project) + + pipeline = HTML::Pipeline.new([described_class, Gitlab::Markdown::ReferenceGathererFilter], contexts) + pipeline.call(body) + end + # Modify a String reference to make it invalid # # Commit SHAs get reversed, IDs get incremented by 1, all other Strings get -- cgit v1.2.1 From 72afcbcd37f35e02544290977c2c91ae37c12c01 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 7 Oct 2015 19:19:23 +0200 Subject: Always allow references to the current project --- app/models/concerns/mentionable.rb | 1 - lib/gitlab/markdown/redactor_filter.rb | 2 +- lib/gitlab/markdown/reference_filter.rb | 7 +++++-- lib/gitlab/markdown/reference_gatherer_filter.rb | 2 +- lib/gitlab/markdown/user_reference_filter.rb | 2 +- spec/lib/gitlab/reference_extractor_spec.rb | 4 ++-- spec/support/mentionable_shared_examples.rb | 2 ++ 7 files changed, 12 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 5b0ae411642..7ad8d5b7da7 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -61,7 +61,6 @@ module Mentionable ext = Gitlab::ReferenceExtractor.new(p, current_user) ext.analyze(text) - (ext.issues + ext.merge_requests + ext.commits).uniq - [local_reference] end diff --git a/lib/gitlab/markdown/redactor_filter.rb b/lib/gitlab/markdown/redactor_filter.rb index 07ea6207d22..a1f3a8a8ebf 100644 --- a/lib/gitlab/markdown/redactor_filter.rb +++ b/lib/gitlab/markdown/redactor_filter.rb @@ -26,7 +26,7 @@ module Gitlab reference_type = node.attr('data-reference-filter') reference_filter = reference_type.constantize - reference_filter.user_can_reference?(current_user, node) + reference_filter.user_can_reference?(current_user, node, context) else true end diff --git a/lib/gitlab/markdown/reference_filter.rb b/lib/gitlab/markdown/reference_filter.rb index ea6136b3303..8ad52479d3d 100644 --- a/lib/gitlab/markdown/reference_filter.rb +++ b/lib/gitlab/markdown/reference_filter.rb @@ -15,9 +15,12 @@ module Gitlab # Results: # :references - A Hash of references that were found and replaced. class ReferenceFilter < HTML::Pipeline::Filter - def self.user_can_reference?(user, node) + def self.user_can_reference?(user, node, context) if node.has_attribute?('data-project') - project = Project.find(node.attr('data-project')) rescue nil + project_id = node.attr('data-project').to_i + return true if project_id == context[:project].id + + project = Project.find(project_id) rescue nil Ability.abilities.allowed?(user, :read_project, project) else true diff --git a/lib/gitlab/markdown/reference_gatherer_filter.rb b/lib/gitlab/markdown/reference_gatherer_filter.rb index d64671e9550..171ff906da6 100644 --- a/lib/gitlab/markdown/reference_gatherer_filter.rb +++ b/lib/gitlab/markdown/reference_gatherer_filter.rb @@ -31,7 +31,7 @@ module Gitlab reference_type = node.attr('data-reference-filter') reference_filter = reference_type.constantize - return unless reference_filter.user_can_reference?(current_user, node) + return unless reference_filter.user_can_reference?(current_user, node, context) references = reference_filter.referenced_by(node) return unless references diff --git a/lib/gitlab/markdown/user_reference_filter.rb b/lib/gitlab/markdown/user_reference_filter.rb index 0d2be7499b7..4567e983692 100644 --- a/lib/gitlab/markdown/user_reference_filter.rb +++ b/lib/gitlab/markdown/user_reference_filter.rb @@ -42,7 +42,7 @@ module Gitlab end end - def self.user_can_reference?(user, node) + def self.user_can_reference?(user, node, context) if node.has_attribute?('data-group') group = Group.find(node.attr('data-group')) rescue nil Ability.abilities.allowed?(user, :read_group, group) diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index 6d7a067e4e0..088e34f050c 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/spec/lib/gitlab/reference_extractor_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::ReferenceExtractor do let(:project) { create(:project) } - subject { Gitlab::ReferenceExtractor.new(project, project.owner) } + subject { Gitlab::ReferenceExtractor.new(project, project.creator) } it 'accesses valid user objects' do @u_foo = create(:user, username: 'foo') @@ -102,7 +102,7 @@ describe Gitlab::ReferenceExtractor do let(:issue) { create(:issue, project: other_project) } before do - other_project.team << [project.owner, :developer] + other_project.team << [project.creator, :developer] end it 'handles project issue references' do diff --git a/spec/support/mentionable_shared_examples.rb b/spec/support/mentionable_shared_examples.rb index e3de0afb448..2b52b4c9b10 100644 --- a/spec/support/mentionable_shared_examples.rb +++ b/spec/support/mentionable_shared_examples.rb @@ -50,6 +50,8 @@ def common_mentionable_setup } extra_commits.each { |c| commitmap[c.short_id] = c } + allow(Project).to receive(:find).and_call_original + allow(Project).to receive(:find).with(project.id.to_s).and_return(project) allow(project.repository).to receive(:commit) { |sha| commitmap[sha] } set_mentionable_text.call(ref_string) -- cgit v1.2.1 From e0f9d3a547e340d229f6164c917d19955a10b371 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 7 Oct 2015 18:25:36 -0400 Subject: Update ReferenceFilter docs [ci skip] --- lib/gitlab/markdown/reference_filter.rb | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/gitlab/markdown/reference_filter.rb b/lib/gitlab/markdown/reference_filter.rb index 8ad52479d3d..d334a891509 100644 --- a/lib/gitlab/markdown/reference_filter.rb +++ b/lib/gitlab/markdown/reference_filter.rb @@ -11,15 +11,12 @@ module Gitlab # Context options: # :project (required) - Current project, ignored if reference is cross-project. # :only_path - Generate path-only links. - # - # Results: - # :references - A Hash of references that were found and replaced. class ReferenceFilter < HTML::Pipeline::Filter def self.user_can_reference?(user, node, context) if node.has_attribute?('data-project') project_id = node.attr('data-project').to_i return true if project_id == context[:project].id - + project = Project.find(project_id) rescue nil Ability.abilities.allowed?(user, :read_project, project) else @@ -33,14 +30,16 @@ module Gitlab # Returns a data attribute String to attach to a reference link # - # id - Object ID - # type - Object type (default: :project) + # attributes - Hash, where the key becomes the data attribute name and the + # value is the data attribute value # # Examples: # - # data_attribute(project: 1) # => "data-reference-filter=\"SomeReferenceFilter\" data-project=\"1\"" - # data_attribute(user: 2) # => "data-reference-filter=\"SomeReferenceFilter\" data-user=\"2\"" - # data_attribute(group: 3) # => "data-reference-filter=\"SomeReferenceFilter\" data-group=\"3\"" + # data_attribute(project: 1, issue: 2) + # # => "data-reference-filter=\"Gitlab::Markdown::SomeReferenceFilter\" data-project=\"1\" data-issue=\"2\"" + # + # data_attribute(project: 3, merge_request: 4) + # # => "data-reference-filter=\"Gitlab::Markdown::SomeReferenceFilter\" data-project=\"3\" data-merge-request=\"4\"" # # Returns a String def data_attribute(attributes = {}) @@ -86,7 +85,7 @@ module Gitlab # Yields the current node's String contents. The result of the block will # replace the node's existing content and update the current document. # - # Returns the updated Nokogiri::XML::Document object. + # Returns the updated Nokogiri::HTML::DocumentFragment object. def replace_text_nodes_matching(pattern) return doc if project.nil? -- cgit v1.2.1 From e6419cecf4221d44be6e80a41ed668bd79f247fb Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 8 Oct 2015 12:58:47 +0200 Subject: Update inline doc --- lib/gitlab/markdown/reference_gatherer_filter.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/markdown/reference_gatherer_filter.rb b/lib/gitlab/markdown/reference_gatherer_filter.rb index 171ff906da6..89acb312cd5 100644 --- a/lib/gitlab/markdown/reference_gatherer_filter.rb +++ b/lib/gitlab/markdown/reference_gatherer_filter.rb @@ -3,8 +3,8 @@ require 'html/pipeline/filter' module Gitlab module Markdown - # HTML filter that removes references to records that the current user does - # not have permission to view. + # HTML filter that gathers all referenced records that the current user has + # permission to view. # # Expected to be run in its own post-processing pipeline. # -- cgit v1.2.1 From 9d066bc9417b06c8eaa8a7e5e73b153093102bbc Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 8 Oct 2015 13:00:15 +0200 Subject: Raise error when a ReferenceFilter doesn't implement referenced_by --- lib/gitlab/markdown/reference_filter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/markdown/reference_filter.rb b/lib/gitlab/markdown/reference_filter.rb index d334a891509..c7d4b15d458 100644 --- a/lib/gitlab/markdown/reference_filter.rb +++ b/lib/gitlab/markdown/reference_filter.rb @@ -25,7 +25,7 @@ module Gitlab end def self.referenced_by(node) - nil + raise NotImplementedError, "#{self} does not implement #{__method__}" end # Returns a data attribute String to attach to a reference link -- cgit v1.2.1 From ae4fbae26cefbf10848719ee8c06d418c348420c Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Thu, 8 Oct 2015 11:13:28 -0400 Subject: Send an email (to support) when a user is reported for spam --- app/controllers/abuse_reports_controller.rb | 3 ++ .../admin/application_settings_controller.rb | 1 + app/mailers/abuse_report_mailer.rb | 8 ++++ app/views/abuse_report_mailer/notify.text.haml | 5 ++ .../admin/application_settings/_form.html.haml | 4 ++ ...8143519_add_admin_notification_email_setting.rb | 5 ++ db/schema.rb | 3 +- spec/controllers/abuse_reports_controller_spec.rb | 53 ++++++++++++++++++++++ 8 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 app/mailers/abuse_report_mailer.rb create mode 100644 app/views/abuse_report_mailer/notify.text.haml create mode 100644 db/migrate/20151008143519_add_admin_notification_email_setting.rb create mode 100644 spec/controllers/abuse_reports_controller_spec.rb diff --git a/app/controllers/abuse_reports_controller.rb b/app/controllers/abuse_reports_controller.rb index 65dbd5ef551..482ec5054ac 100644 --- a/app/controllers/abuse_reports_controller.rb +++ b/app/controllers/abuse_reports_controller.rb @@ -11,6 +11,9 @@ class AbuseReportsController < ApplicationController if @abuse_report.save message = "Thank you for your report. A GitLab administrator will look into it shortly." redirect_to root_path, notice: message + if current_application_settings.admin_notification_email.present? + AbuseReportMailer.delay.notify(@abuse_report, current_application_settings.admin_notification_email) + end else render :new end diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 5f70582cbb7..18a258c139f 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -55,6 +55,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController :default_snippet_visibility, :restricted_signup_domains_raw, :version_check_enabled, + :admin_notification_email, :user_oauth_applications, :ci_enabled, restricted_visibility_levels: [], diff --git a/app/mailers/abuse_report_mailer.rb b/app/mailers/abuse_report_mailer.rb new file mode 100644 index 00000000000..c8b9c9c1628 --- /dev/null +++ b/app/mailers/abuse_report_mailer.rb @@ -0,0 +1,8 @@ +class AbuseReportMailer < BaseMailer + + def notify(abuse_report, to_email) + @abuse_report = abuse_report + + mail(to: to_email, subject: "[Gitlab] Abuse report filed for `#{@abuse_report.user.username}`") + end +end diff --git a/app/views/abuse_report_mailer/notify.text.haml b/app/views/abuse_report_mailer/notify.text.haml new file mode 100644 index 00000000000..70e4e6a3c6c --- /dev/null +++ b/app/views/abuse_report_mailer/notify.text.haml @@ -0,0 +1,5 @@ +An abuse report was filed on `#{@abuse_report.user.username}` by `#{@abuse_report.reporter.username}`. +\ += @abuse_report.message +\ +Abuse report admin screen: #{abuse_reports_url} \ No newline at end of file diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 143cd10c543..036e24d3a46 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -47,6 +47,10 @@ = f.label :version_check_enabled do = f.check_box :version_check_enabled Version check enabled + .form-group + = f.label :admin_notification_email, class: 'control-label col-sm-2' + .col-sm-10 + = f.text_field :admin_notification_email, class: 'form-control' %fieldset %legend Account and Limit Settings diff --git a/db/migrate/20151008143519_add_admin_notification_email_setting.rb b/db/migrate/20151008143519_add_admin_notification_email_setting.rb new file mode 100644 index 00000000000..0bb581efe2c --- /dev/null +++ b/db/migrate/20151008143519_add_admin_notification_email_setting.rb @@ -0,0 +1,5 @@ +class AddAdminNotificationEmailSetting < ActiveRecord::Migration + def change + add_column :application_settings, :admin_notification_email, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 72609da93f1..23627bdaa22 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150930095736) do +ActiveRecord::Schema.define(version: 20151008143519) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -47,6 +47,7 @@ ActiveRecord::Schema.define(version: 20150930095736) do t.text "import_sources" t.text "help_page_text" t.boolean "ci_enabled", default: true, null: false + t.string "admin_notification_email" end create_table "audit_events", force: true do |t| diff --git a/spec/controllers/abuse_reports_controller_spec.rb b/spec/controllers/abuse_reports_controller_spec.rb new file mode 100644 index 00000000000..6d157406a2b --- /dev/null +++ b/spec/controllers/abuse_reports_controller_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe AbuseReportsController do + let(:reporter) { create(:user) } + let(:user) { create(:user) } + let(:message) { "This user is a spammer" } + + before do + sign_in(reporter) + end + + describe "with admin notification_email set" do + let(:admin_email) { "admin@example.com"} + before(:example) { allow(current_application_settings).to receive(:admin_notification_email).and_return(admin_email) } + + it "sends a notification email" do + post(:create, + abuse_report: { + user_id: user.id, + message: message + } + ) + + expect(response).to have_http_status(:redirect) + expect(flash[:notice]).to start_with("Thank you for your report") + + email = ActionMailer::Base.deliveries.last + + expect(email).to be_present + expect(email.subject).to eq("[Gitlab] Abuse report filed for `#{user.username}`") + expect(email.to).to eq([admin_email]) + expect(email.body).to include(message) + end + end + + describe "without admin notification email set" do + before(:example) { allow(current_application_settings).to receive(:admin_notification_email).and_return(nil) } + + it "does not send a notification email" do + expect do + post(:create, + abuse_report: { + user_id: user.id, + message: message + } + ) + end.to_not change{ActionMailer::Base.deliveries} + + expect(response).to have_http_status(:redirect) + expect(flash[:notice]).to start_with("Thank you for your report") + end + end +end \ No newline at end of file -- cgit v1.2.1 From b8f5e7427f7cbcdb19dcc5554301e87a880cfe2a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 12 Oct 2015 12:07:31 +0200 Subject: Show notifications button even if user is not member of a project Notifications button was unavailable if user wasn't member of the project, even if protected project is available via group privileges. Showing disabled button with explanation tool-tip is less confusing. This closes #2846. --- app/controllers/projects_controller.rb | 1 + .../projects/buttons/_notifications.html.haml | 33 +++++++++++++--------- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 213c2a7173b..ffbd91324cb 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -88,6 +88,7 @@ class ProjectsController < ApplicationController else if current_user @membership = @project.project_member_by_id(current_user.id) + @group_member = GroupMember.find_by(user_id: current_user.id) end render :show diff --git a/app/views/projects/buttons/_notifications.html.haml b/app/views/projects/buttons/_notifications.html.haml index 4b69a6d7a6f..501a51d0e8a 100644 --- a/app/views/projects/buttons/_notifications.html.haml +++ b/app/views/projects/buttons/_notifications.html.haml @@ -1,14 +1,21 @@ -- return unless @membership +- return unless [@membership, @group_member].any? -= form_tag profile_notifications_path, method: :put, remote: true, class: 'inline-form', id: 'notification-form' do - = hidden_field_tag :notification_type, 'project' - = hidden_field_tag :notification_id, @membership.id - = hidden_field_tag :notification_level - %span.dropdown - %a.dropdown-new.btn.btn-new#notifications-button{href: '#', "data-toggle" => "dropdown"} - = icon('bell') - = notification_label(@membership) - = icon('angle-down') - %ul.dropdown-menu.dropdown-menu-right.project-home-dropdown - - Notification.project_notification_levels.each do |level| - = notification_list_item(level, @membership) +- if @membership + = form_tag profile_notifications_path, method: :put, remote: true, class: 'inline-form', id: 'notification-form' do + = hidden_field_tag :notification_type, 'project' + = hidden_field_tag :notification_id, @membership.id + = hidden_field_tag :notification_level + %span.dropdown + %a.dropdown-new.btn.btn-new#notifications-button{href: '#', "data-toggle" => "dropdown"} + = icon('bell') + = notification_label(@membership) + = icon('angle-down') + %ul.dropdown-menu.dropdown-menu-right.project-home-dropdown + - Notification.project_notification_levels.each do |level| + = notification_list_item(level, @membership) + +- elsif @group_member + .btn.btn-new.disabled#notifications-button + = icon('bell') + = notification_label(@group_member) + = icon('angle-down') -- cgit v1.2.1 From df99ddbba13db4a7699bf1d585675f921cf382ce Mon Sep 17 00:00:00 2001 From: Han Loong Liauw Date: Tue, 13 Oct 2015 21:24:44 +1100 Subject: Adds ability to remove the forked relationship This was previously possible through the API but can now be done through the project#edit settings screen if the current user is the owner of the project. Update changelog --- CHANGELOG | 2 ++ app/controllers/projects_controller.rb | 9 ++++++- app/helpers/projects_helper.rb | 4 +++ app/models/ability.rb | 3 ++- app/views/projects/edit.html.haml | 16 +++++++++++ app/views/projects/remove_fork.js.haml | 2 ++ config/routes.rb | 1 + lib/api/projects.rb | 2 +- spec/controllers/projects_controller_spec.rb | 40 ++++++++++++++++++++++++++++ spec/features/projects_spec.rb | 29 +++++++++++++++++--- 10 files changed, 101 insertions(+), 7 deletions(-) create mode 100644 app/views/projects/remove_fork.js.haml diff --git a/CHANGELOG b/CHANGELOG index a3d796bea66..3c730aef5e4 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -45,6 +45,8 @@ v 8.1.0 (unreleased) - Fix position of hamburger in header for smaller screens (Han Loong Liauw) - Fix bug where Emojis in Markdown would truncate remaining text (Sakata Sinji) - Persist filters when sorting on admin user page (Jerry Lukins) + - Adds ability to remove the forked relationship from project settings + screen. #2578 (Han Loong Liauw) v 8.0.4 - Fix Message-ID header to be RFC 2111-compliant to prevent e-mails being dropped (Stan Hu) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 213c2a7173b..1fb83d0eb6d 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -5,7 +5,7 @@ class ProjectsController < ApplicationController before_action :repository, except: [:new, :create] # Authorize - before_action :authorize_admin_project!, only: [:edit, :update, :destroy, :transfer, :archive, :unarchive] + before_action :authorize_admin_project!, only: [:edit, :update, :destroy, :transfer, :archive, :unarchive, :remove_fork] before_action :event_filter, only: [:show, :activity] layout :determine_layout @@ -64,6 +64,13 @@ class ProjectsController < ApplicationController end end + def remove_fork + if @project.forked? + @project.forked_project_link.destroy + flash[:notice] = 'Fork relationship has been removed.' + end + end + def activity respond_to do |format| format.html diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index a0220af4c30..b0a3a20fa0a 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -70,6 +70,10 @@ module ProjectsHelper "You are going to transfer #{project.name_with_namespace} to another owner. Are you ABSOLUTELY sure?" end + def remove_fork_project_message(project) + "You are going to remove the fork relationship to the source project from #{@project.forked_from_project.namespace.try(:name)}. Are you ABSOLUTELY sure?" + end + def project_nav_tabs @nav_tabs ||= get_project_nav_tabs(@project, current_user) end diff --git a/app/models/ability.rb b/app/models/ability.rb index a020b24a550..11ada46610b 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -185,7 +185,8 @@ class Ability :change_visibility_level, :rename_project, :remove_project, - :archive_project + :archive_project, + :remove_fork_project ] end diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 1882a82fba5..ce77a9242fc 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -189,6 +189,21 @@ - else .nothing-here-block Only the project owner can transfer a project + - if @project.forked? && can?(current_user, :remove_fork_project, @project) + = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_namespace_project_path(@project.namespace, @project), method: :put, remote: true, html: { class: 'transfer-project form-horizontal' }) do |f| + .panel.panel-default.panel.panel-danger + .panel-heading Remove forked relationship + .panel-body + %p + This will remove the relationship to the source project from + = link_to project_path(@project.forked_from_project) do + = @project.forked_from_project.namespace.try(:name) + %br + %strong Once removed it cannot be reversed through this interface + = button_to 'Remove forked relationship', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) } + - elsif @project.forked? + .nothing-here-block Only the project owner can remove the fork relationship + - if can?(current_user, :remove_project, @project) .panel.panel-default.panel.panel-danger .panel-heading Remove project @@ -203,6 +218,7 @@ - else .nothing-here-block Only project owner can remove a project + .save-project-loader.hide .center %h2 diff --git a/app/views/projects/remove_fork.js.haml b/app/views/projects/remove_fork.js.haml new file mode 100644 index 00000000000..17b9fecfeb1 --- /dev/null +++ b/app/views/projects/remove_fork.js.haml @@ -0,0 +1,2 @@ +:plain + location.href = "#{edit_namespace_project_path(@project.namespace, @project)}"; diff --git a/config/routes.rb b/config/routes.rb index 8e6fbf6340c..3ac4342b23a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -378,6 +378,7 @@ Gitlab::Application.routes.draw do [:new, :create, :index], path: "/") do member do put :transfer + put :remove_fork post :archive post :unarchive post :toggle_star diff --git a/lib/api/projects.rb b/lib/api/projects.rb index c2fb36b4143..e8a0e7f3ec9 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -247,7 +247,7 @@ module API # DELETE /projects/:id/fork delete ":id/fork" do authenticated_as_admin! - unless user_project.forked_project_link.nil? + if user_project.forked? user_project.forked_project_link.destroy end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 21beaf37fce..626b56cc789 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -62,4 +62,44 @@ describe ProjectsController do expect(user.starred?(public_project)).to be_falsey end end + + describe "PUT remove_fork" do + context 'when signed in' do + before do + sign_in(user) + end + + context 'with forked project' do + let(:project_fork) { create(:project, namespace: user.namespace) } + + it 'should remove fork from project' do + create(:forked_project_link, forked_to_project: project_fork) + put(:remove_fork, + namespace_id: project_fork.namespace.to_param, + id: project_fork.to_param, format: :js) + + expect(project_fork.forked?).to be_falsey + expect(flash[:notice]).to eq('Fork relationship has been removed.') + expect(response).to render_template(:remove_fork) + end + end + + it 'should do nothing if project was not forked' do + unforked_project = create(:project, namespace: user.namespace) + put(:remove_fork, + namespace_id: unforked_project.namespace.to_param, + id: unforked_project.to_param, format: :js) + + expect(flash[:notice]).to be_nil + expect(response).to render_template(:remove_fork) + end + end + + it "does nothing if user is not signed in" do + put(:remove_fork, + namespace_id: project.namespace.to_param, + id: project.to_param, format: :js) + expect(response.status).to eq(401) + end + end end diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index aac93b17a38..df0dcb2bb21 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -34,6 +34,27 @@ feature 'Project', feature: true do end end + describe 'remove forked relationship', js: true do + let(:user) { create(:user) } + let(:project) { create(:project, namespace: user.namespace) } + + before do + login_with user + create(:forked_project_link, forked_to_project: project) + visit edit_namespace_project_path(project.namespace, project) + end + + it 'should remove fork' do + expect(page).to have_content 'Remove forked relationship' + + remove_with_confirm('Remove forked relationship', project.path) + + expect(page).to have_content 'Fork relationship has been removed.' + expect(project.forked?).to be_falsey + expect(page).not_to have_content 'Remove forked relationship' + end + end + describe 'removal', js: true do let(:user) { create(:user) } let(:project) { create(:project, namespace: user.namespace) } @@ -45,13 +66,13 @@ feature 'Project', feature: true do end it 'should remove project' do - expect { remove_project }.to change {Project.count}.by(-1) + expect { remove_with_confirm('Remove project', project.path) }.to change {Project.count}.by(-1) end end - def remove_project - click_button "Remove project" - fill_in 'confirm_name_input', with: project.path + def remove_with_confirm(button_text, confirm_with) + click_button button_text + fill_in 'confirm_name_input', with: confirm_with click_button 'Confirm' end end -- cgit v1.2.1 From 5dd77358f6293249494bf390140843f63dfd220a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 13 Oct 2015 13:36:47 +0200 Subject: Pass project to RedactorFilter --- app/helpers/gitlab_markdown_helper.rb | 4 ++-- lib/gitlab/markdown.rb | 11 ++++++++--- lib/gitlab/markdown/reference_filter.rb | 2 +- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 1b8bb46d25e..65813482120 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -59,7 +59,7 @@ module GitlabMarkdownHelper user = current_user if defined?(current_user) html = Gitlab::Markdown.render(text, context) - Gitlab::Markdown.post_process(html, pipeline: context[:pipeline], user: user) + Gitlab::Markdown.post_process(html, pipeline: context[:pipeline], project: @project, user: user) end # TODO (rspeicher): Remove all usages of this helper and just call `markdown` @@ -78,7 +78,7 @@ module GitlabMarkdownHelper user = current_user if defined?(current_user) html = Gitlab::Markdown.gfm(text, options) - Gitlab::Markdown.post_process(html, pipeline: options[:pipeline], user: user) + Gitlab::Markdown.post_process(html, pipeline: options[:pipeline], project: @project, user: user) end def asciidoc(text) diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index f389d0a80dd..d5b0060dd56 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -47,12 +47,17 @@ module Gitlab # # html - String to process # options - Hash of options to customize output - # :pipeline - Symbol pipeline type - # :user - User object + # :pipeline - Symbol pipeline type + # :project - Project + # :user - User object # # Returns an HTML-safe String def self.post_process(html, options) - doc = post_processor.to_document(html, current_user: options[:user]) + context = { + project: options[:project], + current_user: options[:user] + } + doc = post_processor.to_document(html, context) if options[:pipeline] == :atom doc.to_html(save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML) diff --git a/lib/gitlab/markdown/reference_filter.rb b/lib/gitlab/markdown/reference_filter.rb index c7d4b15d458..0ea966c744b 100644 --- a/lib/gitlab/markdown/reference_filter.rb +++ b/lib/gitlab/markdown/reference_filter.rb @@ -15,7 +15,7 @@ module Gitlab def self.user_can_reference?(user, node, context) if node.has_attribute?('data-project') project_id = node.attr('data-project').to_i - return true if project_id == context[:project].id + return true if project_id == context[:project].try(:id) project = Project.find(project_id) rescue nil Ability.abilities.allowed?(user, :read_project, project) -- cgit v1.2.1 From 251e13666d04a1c8427401962e3e171e569d9088 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 13 Oct 2015 18:23:49 +0200 Subject: Efficiently load multiple references of one type. --- lib/gitlab/markdown/issue_reference_filter.rb | 5 +--- lib/gitlab/markdown/label_reference_filter.rb | 5 +--- .../markdown/merge_request_reference_filter.rb | 5 +--- lib/gitlab/markdown/reference_filter.rb | 2 ++ lib/gitlab/markdown/reference_gatherer_filter.rb | 29 +++++++++++++++++++--- lib/gitlab/markdown/snippet_reference_filter.rb | 5 +--- lib/gitlab/markdown/user_reference_filter.rb | 5 +--- 7 files changed, 33 insertions(+), 23 deletions(-) diff --git a/lib/gitlab/markdown/issue_reference_filter.rb b/lib/gitlab/markdown/issue_reference_filter.rb index cd2a9b75680..481d282f7b1 100644 --- a/lib/gitlab/markdown/issue_reference_filter.rb +++ b/lib/gitlab/markdown/issue_reference_filter.rb @@ -28,10 +28,7 @@ module Gitlab end def self.referenced_by(node) - issue = Issue.find(node.attr("data-issue")) rescue nil - return unless issue - - { issue: issue } + { issue: LazyReference.new(Issue, node.attr("data-issue")) } end def call diff --git a/lib/gitlab/markdown/label_reference_filter.rb b/lib/gitlab/markdown/label_reference_filter.rb index 59568ab531f..618acb7a578 100644 --- a/lib/gitlab/markdown/label_reference_filter.rb +++ b/lib/gitlab/markdown/label_reference_filter.rb @@ -23,10 +23,7 @@ module Gitlab end def self.referenced_by(node) - label = Label.find(node.attr("data-label")) rescue nil - return unless label - - { label: label } + { label: LazyReference.new(Label, node.attr("data-label")) } end def call diff --git a/lib/gitlab/markdown/merge_request_reference_filter.rb b/lib/gitlab/markdown/merge_request_reference_filter.rb index 440574e574b..5bc63269808 100644 --- a/lib/gitlab/markdown/merge_request_reference_filter.rb +++ b/lib/gitlab/markdown/merge_request_reference_filter.rb @@ -28,10 +28,7 @@ module Gitlab end def self.referenced_by(node) - merge_request = MergeRequest.find(node.attr("data-merge-request")) rescue nil - return unless merge_request - - { merge_request: merge_request } + { merge_request: LazyReference.new(MergeRequest, node.attr("data-merge-request")) } end def call diff --git a/lib/gitlab/markdown/reference_filter.rb b/lib/gitlab/markdown/reference_filter.rb index 0ea966c744b..0ae0d93f70d 100644 --- a/lib/gitlab/markdown/reference_filter.rb +++ b/lib/gitlab/markdown/reference_filter.rb @@ -12,6 +12,8 @@ module Gitlab # :project (required) - Current project, ignored if reference is cross-project. # :only_path - Generate path-only links. class ReferenceFilter < HTML::Pipeline::Filter + LazyReference = Struct.new(:klass, :ids) + def self.user_can_reference?(user, node, context) if node.has_attribute?('data-project') project_id = node.attr('data-project').to_i diff --git a/lib/gitlab/markdown/reference_gatherer_filter.rb b/lib/gitlab/markdown/reference_gatherer_filter.rb index 89acb312cd5..cf9a2303db8 100644 --- a/lib/gitlab/markdown/reference_gatherer_filter.rb +++ b/lib/gitlab/markdown/reference_gatherer_filter.rb @@ -12,7 +12,8 @@ module Gitlab def initialize(*) super - result[:references] ||= Hash.new { |hash, type| hash[type] = [] } + result[:lazy_references] ||= Hash.new { |hash, type| hash[type] = [] } + result[:references] ||= Hash.new { |hash, type| hash[type] = [] } end def call @@ -20,6 +21,8 @@ module Gitlab gather_references(node) end + load_lazy_references + doc end @@ -35,9 +38,29 @@ module Gitlab references = reference_filter.referenced_by(node) return unless references - + references.each do |type, values| - result[:references][type].push(*values) + Array.wrap(values).each do |value| + refs = + if value.is_a?(ReferenceFilter::LazyReference) + result[:lazy_references] + else + result[:references] + end + + refs[type] << value + end + end + end + + # Will load all references of one type using one query. + def load_lazy_references + result[:lazy_references].each do |type, refs| + refs.group_by(&:klass).each do |klass, refs| + ids = refs.map(&:ids).flatten + values = klass.find(ids) + result[:references][type].push(*values) + end end end diff --git a/lib/gitlab/markdown/snippet_reference_filter.rb b/lib/gitlab/markdown/snippet_reference_filter.rb index a7396e96529..f783f951711 100644 --- a/lib/gitlab/markdown/snippet_reference_filter.rb +++ b/lib/gitlab/markdown/snippet_reference_filter.rb @@ -28,10 +28,7 @@ module Gitlab end def self.referenced_by(node) - snippet = Snippet.find(node.attr("data-snippet")) rescue nil - return unless snippet - - { snippet: snippet } + { snippet: LazyReference.new(Snippet, node.attr("data-snippet")) } end def call diff --git a/lib/gitlab/markdown/user_reference_filter.rb b/lib/gitlab/markdown/user_reference_filter.rb index 4567e983692..2a594e1662e 100644 --- a/lib/gitlab/markdown/user_reference_filter.rb +++ b/lib/gitlab/markdown/user_reference_filter.rb @@ -30,10 +30,7 @@ module Gitlab { user: group.users } elsif node.has_attribute?('data-user') - user = User.find(node.attr('data-user')) rescue nil - return unless user - - { user: user } + { user: LazyReference.new(User, node.attr('data-user')) } elsif node.has_attribute?('data-project') project = Project.find(node.attr('data-project')) rescue nil return unless project -- cgit v1.2.1 From 93fcddd7a7ce4ed259794a4511ae04035ae33be2 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 13 Oct 2015 22:53:05 +0200 Subject: Allow ReferenceExtractor to efficiently load references from multiple texts at once --- lib/gitlab/markdown/reference_filter.rb | 9 ++++++++- lib/gitlab/markdown/reference_gatherer_filter.rb | 9 +++------ lib/gitlab/reference_extractor.rb | 23 ++++++++++++++++++----- spec/lib/gitlab/reference_extractor_spec.rb | 14 +++++++------- 4 files changed, 36 insertions(+), 19 deletions(-) diff --git a/lib/gitlab/markdown/reference_filter.rb b/lib/gitlab/markdown/reference_filter.rb index 0ae0d93f70d..ede9f8865ff 100644 --- a/lib/gitlab/markdown/reference_filter.rb +++ b/lib/gitlab/markdown/reference_filter.rb @@ -12,7 +12,14 @@ module Gitlab # :project (required) - Current project, ignored if reference is cross-project. # :only_path - Generate path-only links. class ReferenceFilter < HTML::Pipeline::Filter - LazyReference = Struct.new(:klass, :ids) + LazyReference = Struct.new(:klass, :ids) do + def self.load(refs) + refs.group_by(&:klass).flat_map do |klass, refs| + ids = refs.flat_map(&:ids) + klass.where(id: ids) + end + end + end def self.user_can_reference?(user, node, context) if node.has_attribute?('data-project') diff --git a/lib/gitlab/markdown/reference_gatherer_filter.rb b/lib/gitlab/markdown/reference_gatherer_filter.rb index cf9a2303db8..18df5db94d6 100644 --- a/lib/gitlab/markdown/reference_gatherer_filter.rb +++ b/lib/gitlab/markdown/reference_gatherer_filter.rb @@ -21,7 +21,7 @@ module Gitlab gather_references(node) end - load_lazy_references + load_lazy_references unless context[:load_lazy_references] == false doc end @@ -56,11 +56,8 @@ module Gitlab # Will load all references of one type using one query. def load_lazy_references result[:lazy_references].each do |type, refs| - refs.group_by(&:klass).each do |klass, refs| - ids = refs.map(&:ids).flatten - values = klass.find(ids) - result[:references][type].push(*values) - end + values = ReferenceFilter::LazyReference.load(refs) + result[:references][type].concat(values) end end diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 2e546ef0d54..8100f2675a7 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -10,9 +10,10 @@ module Gitlab @current_user = current_user end - def analyze(text) + def analyze(texts) references.clear - @text = Gitlab::Markdown.render_without_gfm(text) + texts = Array(texts) + @texts = texts.map { |text| Gitlab::Markdown.render_without_gfm(text) } end %i(user label issue merge_request snippet commit commit_range).each do |type| @@ -47,13 +48,25 @@ module Gitlab current_user: current_user, # We don't actually care about the links generated only_path: true, - ignore_blockquotes: true + ignore_blockquotes: true, + load_lazy_references: false } pipeline = HTML::Pipeline.new([filter, Gitlab::Markdown::ReferenceGathererFilter], context) - result = pipeline.call(@text) - result[:references][filter_type] + values = [] + lazy_references = [] + + @texts.each do |text| + result = pipeline.call(text) + + values.concat(result[:references][filter_type]) + lazy_references.concat(result[:lazy_references][filter_type]) + end + + lazy_values = Gitlab::Markdown::ReferenceFilter::LazyReference.load(lazy_references) + values.concat(lazy_values) + values end end end diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index 088e34f050c..ad84d2274e8 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/spec/lib/gitlab/reference_extractor_spec.rb @@ -13,7 +13,7 @@ describe Gitlab::ReferenceExtractor do project.team << [@u_bar, :guest] subject.analyze('@foo, @baduser, @bar, and @offteam') - expect(subject.users).to eq([@u_foo, @u_bar, @u_offteam]) + expect(subject.users).to match_array([@u_foo, @u_bar, @u_offteam]) end it 'ignores user mentions inside specific elements' do @@ -37,7 +37,7 @@ describe Gitlab::ReferenceExtractor do > @offteam }) - expect(subject.users).to eq([]) + expect(subject.users).to match_array([]) end it 'accesses valid issue objects' do @@ -45,7 +45,7 @@ describe Gitlab::ReferenceExtractor do @i1 = create(:issue, project: project) subject.analyze("#{@i0.to_reference}, #{@i1.to_reference}, and #{Issue.reference_prefix}999.") - expect(subject.issues).to eq([@i0, @i1]) + expect(subject.issues).to match_array([@i0, @i1]) end it 'accesses valid merge requests' do @@ -53,7 +53,7 @@ describe Gitlab::ReferenceExtractor do @m1 = create(:merge_request, source_project: project, target_project: project, source_branch: 'feature_conflict') subject.analyze("!999, !#{@m1.iid}, and !#{@m0.iid}.") - expect(subject.merge_requests).to eq([@m1, @m0]) + expect(subject.merge_requests).to match_array([@m1, @m0]) end it 'accesses valid labels' do @@ -62,7 +62,7 @@ describe Gitlab::ReferenceExtractor do @l2 = create(:label) subject.analyze("~#{@l0.id}, ~999, ~#{@l2.id}, ~#{@l1.id}") - expect(subject.labels).to eq([@l0, @l1]) + expect(subject.labels).to match_array([@l0, @l1]) end it 'accesses valid snippets' do @@ -71,7 +71,7 @@ describe Gitlab::ReferenceExtractor do @s2 = create(:project_snippet) subject.analyze("$#{@s0.id}, $999, $#{@s2.id}, $#{@s1.id}") - expect(subject.snippets).to eq([@s0, @s1]) + expect(subject.snippets).to match_array([@s0, @s1]) end it 'accesses valid commits' do @@ -109,7 +109,7 @@ describe Gitlab::ReferenceExtractor do subject.analyze("this refers issue #{issue.to_reference(project)}") extracted = subject.issues expect(extracted.size).to eq(1) - expect(extracted).to eq([issue]) + expect(extracted).to match_array([issue]) end end end -- cgit v1.2.1 From cd2583a3beed95a91eddf4e6f868507dcf499481 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 13 Oct 2015 23:03:53 +0200 Subject: Code cleanup --- lib/gitlab/markdown/reference_filter.rb | 10 +++++++++- lib/gitlab/markdown/reference_gatherer_filter.rb | 18 +++++------------- lib/gitlab/reference_extractor.rb | 12 +++--------- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/lib/gitlab/markdown/reference_filter.rb b/lib/gitlab/markdown/reference_filter.rb index ede9f8865ff..adaca78ba27 100644 --- a/lib/gitlab/markdown/reference_filter.rb +++ b/lib/gitlab/markdown/reference_filter.rb @@ -14,10 +14,18 @@ module Gitlab class ReferenceFilter < HTML::Pipeline::Filter LazyReference = Struct.new(:klass, :ids) do def self.load(refs) - refs.group_by(&:klass).flat_map do |klass, refs| + lazy_references, values = refs.partition { |ref| ref.is_a?(self) } + + lazy_values = lazy_references.group_by(&:klass).flat_map do |klass, refs| ids = refs.flat_map(&:ids) klass.where(id: ids) end + + values + lazy_values + end + + def load + self.klass.where(id: self.ids) end end diff --git a/lib/gitlab/markdown/reference_gatherer_filter.rb b/lib/gitlab/markdown/reference_gatherer_filter.rb index 18df5db94d6..31fb71a98a3 100644 --- a/lib/gitlab/markdown/reference_gatherer_filter.rb +++ b/lib/gitlab/markdown/reference_gatherer_filter.rb @@ -12,8 +12,7 @@ module Gitlab def initialize(*) super - result[:lazy_references] ||= Hash.new { |hash, type| hash[type] = [] } - result[:references] ||= Hash.new { |hash, type| hash[type] = [] } + result[:references] ||= Hash.new { |hash, type| hash[type] = [] } end def call @@ -41,23 +40,16 @@ module Gitlab references.each do |type, values| Array.wrap(values).each do |value| - refs = - if value.is_a?(ReferenceFilter::LazyReference) - result[:lazy_references] - else - result[:references] - end - - refs[type] << value + result[:references][type] << value end end end # Will load all references of one type using one query. def load_lazy_references - result[:lazy_references].each do |type, refs| - values = ReferenceFilter::LazyReference.load(refs) - result[:references][type].concat(values) + refs = result[:references] + refs.each do |type, values| + refs[type] = ReferenceFilter::LazyReference.load(values) end end diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 8100f2675a7..d6b739d7b9a 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -54,19 +54,13 @@ module Gitlab pipeline = HTML::Pipeline.new([filter, Gitlab::Markdown::ReferenceGathererFilter], context) - values = [] - lazy_references = [] - - @texts.each do |text| + values = @texts.flat_map do |text| result = pipeline.call(text) - values.concat(result[:references][filter_type]) - lazy_references.concat(result[:lazy_references][filter_type]) + result[:references][filter_type] end - lazy_values = Gitlab::Markdown::ReferenceFilter::LazyReference.load(lazy_references) - values.concat(lazy_values) - values + Gitlab::Markdown::ReferenceFilter::LazyReference.load(values) end end end -- cgit v1.2.1 From d6fb96b9276d1a9edfae261d2eba2f79f8a9f340 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 14 Oct 2015 09:17:05 +0200 Subject: Have Issue#participants load all users mentioned in notes using a single query --- app/models/concerns/mentionable.rb | 8 ++++---- app/models/concerns/participable.rb | 23 +++++++++++++---------- lib/gitlab/reference_extractor.rb | 21 +++++++++++---------- 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 7ad8d5b7da7..9339ecc4bce 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -47,19 +47,19 @@ module Mentionable SystemNoteService.cross_reference_exists?(target, local_reference) end - def mentioned_users(current_user = nil) + def mentioned_users(current_user = nil, load_lazy_references: true) return [] if mentionable_text.blank? - ext = Gitlab::ReferenceExtractor.new(self.project, current_user) + ext = Gitlab::ReferenceExtractor.new(self.project, current_user, load_lazy_references: load_lazy_references) ext.analyze(mentionable_text) ext.users.uniq end # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference. - def references(p = project, current_user = self.author, text = mentionable_text) + def references(p = project, current_user = self.author, text = mentionable_text, load_lazy_references: true) return [] if text.blank? - ext = Gitlab::ReferenceExtractor.new(p, current_user) + ext = Gitlab::ReferenceExtractor.new(p, current_user, load_lazy_references: load_lazy_references) ext.analyze(text) (ext.issues + ext.merge_requests + ext.commits).uniq - [local_reference] end diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index 7c9597333dd..7a2bea567df 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -27,7 +27,7 @@ module Participable module ClassMethods def participant(*attrs) - participant_attrs.concat(attrs.map(&:to_s)) + participant_attrs.concat(attrs) end def participant_attrs @@ -37,13 +37,12 @@ module Participable # Be aware that this method makes a lot of sql queries. # Save result into variable if you are going to reuse it inside same request - def participants(current_user = self.author, project = self.project) + def participants(current_user = self.author, project = self.project, load_lazy_references: true) participants = self.class.participant_attrs.flat_map do |attr| meth = method(attr) - value = - if meth.arity == 1 || meth.arity == -1 - meth.call(current_user) + if attr == :mentioned_users + meth.call(current_user, load_lazy_references: false) else meth.call end @@ -51,9 +50,13 @@ module Participable participants_for(value, current_user, project) end.compact.uniq - if project - participants.select! do |user| - user.can?(:read_project, project) + if load_lazy_references + participants = Gitlab::Markdown::ReferenceFilter::LazyReference.load(participants).uniq + + if project + participants.select! do |user| + user.can?(:read_project, project) + end end end @@ -64,12 +67,12 @@ module Participable def participants_for(value, current_user = nil, project = nil) case value - when User + when User, Gitlab::Markdown::ReferenceFilter::LazyReference [value] when Enumerable, ActiveRecord::Relation value.flat_map { |v| participants_for(v, current_user, project) } when Participable - value.participants(current_user, project) + value.participants(current_user, project, load_lazy_references: false) end end end diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index d6b739d7b9a..895a23ddcc8 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -3,17 +3,17 @@ require 'gitlab/markdown' module Gitlab # Extract possible GFM references from an arbitrary String for further processing. class ReferenceExtractor - attr_accessor :project, :current_user + attr_accessor :project, :current_user, :load_lazy_references - def initialize(project, current_user = nil) + def initialize(project, current_user = nil, load_lazy_references: true) @project = project @current_user = current_user + @load_lazy_references = load_lazy_references end - def analyze(texts) + def analyze(text) references.clear - texts = Array(texts) - @texts = texts.map { |text| Gitlab::Markdown.render_without_gfm(text) } + @text = Gitlab::Markdown.render_without_gfm(text) end %i(user label issue merge_request snippet commit commit_range).each do |type| @@ -29,7 +29,7 @@ module Gitlab type = type.to_sym return references[type] if references.has_key?(type) - references[type] = pipeline_result(type).uniq + references[type] = pipeline_result(type) end end @@ -53,14 +53,15 @@ module Gitlab } pipeline = HTML::Pipeline.new([filter, Gitlab::Markdown::ReferenceGathererFilter], context) + result = pipeline.call(@text) - values = @texts.flat_map do |text| - result = pipeline.call(text) + values = result[:references][filter_type].uniq - result[:references][filter_type] + if @load_lazy_references + values = Gitlab::Markdown::ReferenceFilter::LazyReference.load(values).uniq end - Gitlab::Markdown::ReferenceFilter::LazyReference.load(values) + values end end end -- cgit v1.2.1 From d313a7681be1f307d8178eca13a2e73118f80930 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 14 Oct 2015 09:46:06 +0200 Subject: Explicitly only parse references by specified filter --- lib/gitlab/markdown/reference_gatherer_filter.rb | 2 ++ lib/gitlab/reference_extractor.rb | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/markdown/reference_gatherer_filter.rb b/lib/gitlab/markdown/reference_gatherer_filter.rb index 31fb71a98a3..00f983675e6 100644 --- a/lib/gitlab/markdown/reference_gatherer_filter.rb +++ b/lib/gitlab/markdown/reference_gatherer_filter.rb @@ -33,6 +33,8 @@ module Gitlab reference_type = node.attr('data-reference-filter') reference_filter = reference_type.constantize + return if context[:reference_filter] && reference_filter != context[:reference_filter] + return unless reference_filter.user_can_reference?(current_user, node, context) references = reference_filter.referenced_by(node) diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 895a23ddcc8..66016ecc877 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -40,16 +40,20 @@ module Gitlab # # Returns the results Array for the requested filter type def pipeline_result(filter_type) - klass = filter_type.to_s.camelize + 'ReferenceFilter' + klass = "#{filter_type.to_s.camelize}ReferenceFilter" filter = Gitlab::Markdown.const_get(klass) context = { project: project, current_user: current_user, + # We don't actually care about the links generated only_path: true, ignore_blockquotes: true, - load_lazy_references: false + + # ReferenceGathererFilter + load_lazy_references: false, + reference_filter: filter } pipeline = HTML::Pipeline.new([filter, Gitlab::Markdown::ReferenceGathererFilter], context) -- cgit v1.2.1 From 0bea5ced8bf4c9306f8f8e912313731a43d16f4c Mon Sep 17 00:00:00 2001 From: Han Loong Liauw Date: Wed, 14 Oct 2015 09:04:22 +1100 Subject: Made suggested content changes based on MR Review Changed the authentication method for removing fork through API Reflected changes to new auth method in API specs --- CHANGELOG | 2 +- app/controllers/projects_controller.rb | 6 +++- app/views/projects/edit.html.haml | 8 ++--- config/routes.rb | 2 +- lib/api/projects.rb | 2 +- spec/controllers/projects_controller_spec.rb | 26 +++++++++------ spec/features/projects_spec.rb | 6 ++-- spec/requests/api/projects_spec.rb | 50 ++++++++++++++++++---------- 8 files changed, 63 insertions(+), 39 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index cf9e5b43c4d..e45fd2ef5a1 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -46,7 +46,7 @@ v 8.1.0 (unreleased) - Fix bug where Emojis in Markdown would truncate remaining text (Sakata Sinji) - Persist filters when sorting on admin user page (Jerry Lukins) - Adds ability to remove the forked relationship from project settings - screen. #2578 (Han Loong Liauw) + screen. (Han Loong Liauw) - Add spellcheck=false to certain input fields - Invalidate stored service password if the endpoint URL is changed diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 1fb83d0eb6d..77b3af9a5d0 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -5,7 +5,7 @@ class ProjectsController < ApplicationController before_action :repository, except: [:new, :create] # Authorize - before_action :authorize_admin_project!, only: [:edit, :update, :destroy, :transfer, :archive, :unarchive, :remove_fork] + before_action :authorize_admin_project!, only: [:edit, :update] before_action :event_filter, only: [:show, :activity] layout :determine_layout @@ -56,6 +56,8 @@ class ProjectsController < ApplicationController end def transfer + return access_denied! unless can?(current_user, :change_namespace, @project) + namespace = Namespace.find_by(id: params[:new_namespace_id]) ::Projects::TransferService.new(project, current_user).execute(namespace) @@ -65,6 +67,8 @@ class ProjectsController < ApplicationController end def remove_fork + return access_denied! unless can?(current_user, :remove_fork_project, @project) + if @project.forked? @project.forked_project_link.destroy flash[:notice] = 'Fork relationship has been removed.' diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index ce77a9242fc..ec58d0924b0 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -190,17 +190,17 @@ .nothing-here-block Only the project owner can transfer a project - if @project.forked? && can?(current_user, :remove_fork_project, @project) - = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_namespace_project_path(@project.namespace, @project), method: :put, remote: true, html: { class: 'transfer-project form-horizontal' }) do |f| + = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_namespace_project_path(@project.namespace, @project), method: :delete, remote: true, html: { class: 'transfer-project form-horizontal' }) do |f| .panel.panel-default.panel.panel-danger - .panel-heading Remove forked relationship + .panel-heading Remove fork relationship .panel-body %p This will remove the relationship to the source project from = link_to project_path(@project.forked_from_project) do = @project.forked_from_project.namespace.try(:name) %br - %strong Once removed it cannot be reversed through this interface - = button_to 'Remove forked relationship', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) } + %strong Once removed it cannot be reversed through this interface. + = button_to 'Remove fork relationship', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) } - elsif @project.forked? .nothing-here-block Only the project owner can remove the fork relationship diff --git a/config/routes.rb b/config/routes.rb index 3ac4342b23a..64bdd189f53 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -378,7 +378,7 @@ Gitlab::Application.routes.draw do [:new, :create, :index], path: "/") do member do put :transfer - put :remove_fork + delete :remove_fork post :archive post :unarchive post :toggle_star diff --git a/lib/api/projects.rb b/lib/api/projects.rb index e8a0e7f3ec9..67ee66a2058 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -246,7 +246,7 @@ module API # Example Request: # DELETE /projects/:id/fork delete ":id/fork" do - authenticated_as_admin! + authorize! :remove_fork_project, user_project if user_project.forked? user_project.forked_project_link.destroy end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 626b56cc789..e963e913512 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -72,9 +72,12 @@ describe ProjectsController do context 'with forked project' do let(:project_fork) { create(:project, namespace: user.namespace) } - it 'should remove fork from project' do + before do create(:forked_project_link, forked_to_project: project_fork) - put(:remove_fork, + end + + it 'should remove fork from project' do + delete(:remove_fork, namespace_id: project_fork.namespace.to_param, id: project_fork.to_param, format: :js) @@ -84,19 +87,22 @@ describe ProjectsController do end end - it 'should do nothing if project was not forked' do - unforked_project = create(:project, namespace: user.namespace) - put(:remove_fork, - namespace_id: unforked_project.namespace.to_param, - id: unforked_project.to_param, format: :js) + context 'when project not forked' do + let(:unforked_project) { create(:project, namespace: user.namespace) } - expect(flash[:notice]).to be_nil - expect(response).to render_template(:remove_fork) + it 'should do nothing if project was not forked' do + delete(:remove_fork, + namespace_id: unforked_project.namespace.to_param, + id: unforked_project.to_param, format: :js) + + expect(flash[:notice]).to be_nil + expect(response).to render_template(:remove_fork) + end end end it "does nothing if user is not signed in" do - put(:remove_fork, + delete(:remove_fork, namespace_id: project.namespace.to_param, id: project.to_param, format: :js) expect(response.status).to eq(401) diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index df0dcb2bb21..f3d51641ece 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -45,13 +45,13 @@ feature 'Project', feature: true do end it 'should remove fork' do - expect(page).to have_content 'Remove forked relationship' + expect(page).to have_content 'Remove fork relationship' - remove_with_confirm('Remove forked relationship', project.path) + remove_with_confirm('Remove fork relationship', project.path) expect(page).to have_content 'Fork relationship has been removed.' expect(project.forked?).to be_falsey - expect(page).not_to have_content 'Remove forked relationship' + expect(page).not_to have_content 'Remove fork relationship' end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 580bbec77d1..e9de9e0826d 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -606,28 +606,42 @@ describe API::API, api: true do describe 'DELETE /projects/:id/fork' do - it "shouldn't available for non admin users" do + it "shouldn't be visible to users outside group" do delete api("/projects/#{project_fork_target.id}/fork", user) - expect(response.status).to eq(403) + expect(response.status).to eq(404) end - it 'should make forked project unforked' do - post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin) - project_fork_target.reload - expect(project_fork_target.forked_from_project).not_to be_nil - expect(project_fork_target.forked?).to be_truthy - delete api("/projects/#{project_fork_target.id}/fork", admin) - expect(response.status).to eq(200) - project_fork_target.reload - expect(project_fork_target.forked_from_project).to be_nil - expect(project_fork_target.forked?).not_to be_truthy - end + context 'when users belong to project group' do + let(:project_fork_target) { create(:project, group: create(:group)) } - it 'should be idempotent if not forked' do - expect(project_fork_target.forked_from_project).to be_nil - delete api("/projects/#{project_fork_target.id}/fork", admin) - expect(response.status).to eq(200) - expect(project_fork_target.reload.forked_from_project).to be_nil + before do + project_fork_target.group.add_owner user + project_fork_target.group.add_developer user2 + end + + it 'should be forbidden to non-owner users' do + delete api("/projects/#{project_fork_target.id}/fork", user2) + expect(response.status).to eq(403) + end + + it 'should make forked project unforked' do + post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin) + project_fork_target.reload + expect(project_fork_target.forked_from_project).not_to be_nil + expect(project_fork_target.forked?).to be_truthy + delete api("/projects/#{project_fork_target.id}/fork", admin) + expect(response.status).to eq(200) + project_fork_target.reload + expect(project_fork_target.forked_from_project).to be_nil + expect(project_fork_target.forked?).not_to be_truthy + end + + it 'should be idempotent if not forked' do + expect(project_fork_target.forked_from_project).to be_nil + delete api("/projects/#{project_fork_target.id}/fork", admin) + expect(response.status).to eq(200) + expect(project_fork_target.reload.forked_from_project).to be_nil + end end end end -- cgit v1.2.1 From 3a69dd20a1d8e54c25f871f35f09a1b8b5d4b2a4 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 14 Oct 2015 12:33:39 +0200 Subject: Allow dashboard and group issues/MRs to be filtered by label --- CHANGELOG | 1 + app/helpers/labels_helper.rb | 18 +++++++++++++----- app/models/group_label.rb | 9 +++++++++ app/models/group_milestone.rb | 12 ++---------- app/services/labels/group_service.rb | 26 ++++++++++++++++++++++++++ app/views/shared/issuable/_filter.html.haml | 9 ++++----- 6 files changed, 55 insertions(+), 20 deletions(-) create mode 100644 app/models/group_label.rb create mode 100644 app/services/labels/group_service.rb diff --git a/CHANGELOG b/CHANGELOG index a3d796bea66..0a0afb4a053 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -45,6 +45,7 @@ v 8.1.0 (unreleased) - Fix position of hamburger in header for smaller screens (Han Loong Liauw) - Fix bug where Emojis in Markdown would truncate remaining text (Sakata Sinji) - Persist filters when sorting on admin user page (Jerry Lukins) + - Allow dashboard and group issues/MRs to be filtered by label v 8.0.4 - Fix Message-ID header to be RFC 2111-compliant to prevent e-mails being dropped (Stan Hu) diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb index 66b18eea699..ee04ace35d0 100644 --- a/app/helpers/labels_helper.rb +++ b/app/helpers/labels_helper.rb @@ -92,11 +92,19 @@ module LabelsHelper end end - def project_labels_options(project) - labels = project.labels.to_a - labels.unshift(Label::None) - labels.unshift(Label::Any) - options_from_collection_for_select(labels, 'name', 'title', params[:label_name]) + def projects_labels_options + labels = + if @project + @project.labels + else + Label.where(project_id: @projects) + end + + grouped_labels = Labels::GroupService.new(labels).execute + grouped_labels.unshift(Label::None) + grouped_labels.unshift(Label::Any) + + options_from_collection_for_select(grouped_labels, 'name', 'title', params[:label_name]) end # Required for Gitlab::Markdown::LabelReferenceFilter diff --git a/app/models/group_label.rb b/app/models/group_label.rb new file mode 100644 index 00000000000..0fc39cb8771 --- /dev/null +++ b/app/models/group_label.rb @@ -0,0 +1,9 @@ +class GroupLabel + attr_accessor :title, :labels + alias_attribute :name, :title + + def initialize(title, labels) + @title = title + @labels = labels + end +end diff --git a/app/models/group_milestone.rb b/app/models/group_milestone.rb index 1dd2be68ebf..91844da62e2 100644 --- a/app/models/group_milestone.rb +++ b/app/models/group_milestone.rb @@ -1,5 +1,5 @@ class GroupMilestone - + attr_accessor :title, :milestones alias_attribute :name, :title def initialize(title, milestones) @@ -7,18 +7,10 @@ class GroupMilestone @milestones = milestones end - def title - @title - end - def safe_title @title.parameterize end - - def milestones - @milestones - end - + def projects milestones.map { |milestone| milestone.project } end diff --git a/app/services/labels/group_service.rb b/app/services/labels/group_service.rb new file mode 100644 index 00000000000..b26cee24d56 --- /dev/null +++ b/app/services/labels/group_service.rb @@ -0,0 +1,26 @@ +module Labels + class GroupService < ::BaseService + def initialize(project_labels) + @project_labels = project_labels.group_by(&:title) + end + + def execute + build(@project_labels) + end + + def label(title) + if title + group_label = @project_labels[title].group_by(&:title) + build(group_label).first + else + nil + end + end + + private + + def build(label) + label.map { |title, labels| GroupLabel.new(title, labels) } + end + end +end diff --git a/app/views/shared/issuable/_filter.html.haml b/app/views/shared/issuable/_filter.html.haml index 8f16773077e..0e4e9c0987a 100644 --- a/app/views/shared/issuable/_filter.html.haml +++ b/app/views/shared/issuable/_filter.html.haml @@ -42,11 +42,10 @@ class: 'select2 trigger-submit', include_blank: true, data: {placeholder: 'Milestone'}) - - if @project - .filter-item.inline.labels-filter - = select_tag('label_name', project_labels_options(@project), - class: 'select2 trigger-submit', include_blank: true, - data: {placeholder: 'Label'}) + .filter-item.inline.labels-filter + = select_tag('label_name', projects_labels_options, + class: 'select2 trigger-submit', include_blank: true, + data: {placeholder: 'Label'}) .pull-right = render 'shared/sort_dropdown' -- cgit v1.2.1 From 61d8f9617681973f04d264762b54840d44dea02a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 14 Oct 2015 13:46:17 +0200 Subject: Fix specs --- spec/lib/gitlab/closing_issue_extractor_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/lib/gitlab/closing_issue_extractor_spec.rb b/spec/lib/gitlab/closing_issue_extractor_spec.rb index 5d7ff4f6122..21254f778d3 100644 --- a/spec/lib/gitlab/closing_issue_extractor_spec.rb +++ b/spec/lib/gitlab/closing_issue_extractor_spec.rb @@ -140,28 +140,28 @@ describe Gitlab::ClosingIssueExtractor do message = "Closes #{reference} and fix #{reference2}" expect(subject.closed_by_message(message)). - to eq([issue, other_issue]) + to match_array([issue, other_issue]) end it 'fetches comma-separated issues references in single line message' do message = "Closes #{reference}, closes #{reference2}" expect(subject.closed_by_message(message)). - to eq([issue, other_issue]) + to match_array([issue, other_issue]) end it 'fetches comma-separated issues numbers in single line message' do message = "Closes #{reference}, #{reference2} and #{reference3}" expect(subject.closed_by_message(message)). - to eq([issue, other_issue, third_issue]) + to match_array([issue, other_issue, third_issue]) end it 'fetches issues in multi-line message' do message = "Awesome commit (closes #{reference})\nAlso fixes #{reference2}" expect(subject.closed_by_message(message)). - to eq([issue, other_issue]) + to match_array([issue, other_issue]) end it 'fetches issues in hybrid message' do @@ -169,7 +169,7 @@ describe Gitlab::ClosingIssueExtractor do "Also fixing issues #{reference2}, #{reference3} and #4" expect(subject.closed_by_message(message)). - to eq([issue, other_issue, third_issue]) + to match_array([issue, other_issue, third_issue]) end end end -- cgit v1.2.1 From 4a5b77188ec7525d1c3a1ee925c8791f841b040c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 14 Oct 2015 16:20:11 +0200 Subject: Participable doesn't need to know about Mentionable --- app/models/commit.rb | 4 ++-- app/models/concerns/issuable.rb | 4 ++-- app/models/concerns/mentionable.rb | 6 ++++++ app/models/concerns/participable.rb | 9 ++++----- app/models/note.rb | 4 ++-- 5 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index aff329d71fa..95ac7156bed 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -2,13 +2,13 @@ class Commit extend ActiveModel::Naming include ActiveModel::Conversion - include Mentionable include Participable + include Mentionable include Referable include StaticModel attr_mentionable :safe_message - participant :author, :committer, :notes, :mentioned_users + participant :author, :committer, :notes attr_accessor :project diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 4db4ffb2e79..feee8460b86 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -6,8 +6,8 @@ # module Issuable extend ActiveSupport::Concern - include Mentionable include Participable + include Mentionable included do belongs_to :author, class_name: "User" @@ -47,7 +47,7 @@ module Issuable prefix: true attr_mentionable :title, :description - participant :author, :assignee, :notes, :mentioned_users + participant :author, :assignee, :notes end module ClassMethods diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 9339ecc4bce..715fc6f689d 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -20,6 +20,12 @@ module Mentionable end end + included do + if self < Participable + participant ->(current_user) { mentioned_users(current_user, load_lazy_references: false) } + end + end + # Returns the text used as the body of a Note when this object is referenced # # By default this will be the class name and the result of calling diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index 7a2bea567df..0888de62f0a 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -12,7 +12,7 @@ # # # ... # -# participant :author, :assignee, :mentioned_users, :notes +# participant :author, :assignee, :notes, ->(current_user) { mentioned_users(current_user) } # end # # issue = Issue.last @@ -39,12 +39,11 @@ module Participable # Save result into variable if you are going to reuse it inside same request def participants(current_user = self.author, project = self.project, load_lazy_references: true) participants = self.class.participant_attrs.flat_map do |attr| - meth = method(attr) value = - if attr == :mentioned_users - meth.call(current_user, load_lazy_references: false) + if attr.respond_to?(:call) + instance_exec(current_user, &attr) else - meth.call + send(attr) end participants_for(value, current_user, project) diff --git a/app/models/note.rb b/app/models/note.rb index de3b6df88f7..2fbe4784159 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -22,14 +22,14 @@ require 'carrierwave/orm/activerecord' require 'file_size_validator' class Note < ActiveRecord::Base - include Mentionable include Gitlab::CurrentSettings include Participable + include Mentionable default_value_for :system, false attr_mentionable :note - participant :author, :mentioned_users + participant :author belongs_to :project belongs_to :noteable, polymorphic: true -- cgit v1.2.1 From ddeb766e9638d14dd86d966ef097fbeebb9dffab Mon Sep 17 00:00:00 2001 From: Mike Kelly Date: Wed, 14 Oct 2015 18:42:59 +0000 Subject: Remove some padding from code blocks This creates a weird "leading indent" on the first line of code blocks, at least in Chrome 46, and ends up making the first row not line up nicely with everything else. --- app/assets/stylesheets/framework/typography.scss | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index bf36f96cc97..b56758a97d3 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -17,7 +17,6 @@ font-family: $monospace_font; white-space: pre; word-wrap: normal; - padding: 1px 2px; } kbd { @@ -268,4 +267,4 @@ textarea.js-gfm-input { .strikethrough { text-decoration: line-through; -} +} \ No newline at end of file -- cgit v1.2.1 From f7e0357840bb0780ca3ba997aa1e2f4c045f08c7 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 15 Oct 2015 13:22:28 +0200 Subject: Track compatible gitlab-git-http-server version --- GITLAB_GIT_HTTP_SERVER_VERSION | 1 + doc/install/installation.md | 1 + doc/update/7.14-to-8.0.md | 1 + 3 files changed, 3 insertions(+) create mode 100644 GITLAB_GIT_HTTP_SERVER_VERSION diff --git a/GITLAB_GIT_HTTP_SERVER_VERSION b/GITLAB_GIT_HTTP_SERVER_VERSION new file mode 100644 index 00000000000..769ed6ae790 --- /dev/null +++ b/GITLAB_GIT_HTTP_SERVER_VERSION @@ -0,0 +1 @@ +0.2.14 diff --git a/doc/install/installation.md b/doc/install/installation.md index 3c62b11988e..acc4e505971 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -325,6 +325,7 @@ GitLab Shell is an SSH access and repository management software developed speci cd /home/git sudo -u git -H git clone https://gitlab.com/gitlab-org/gitlab-git-http-server.git cd gitlab-git-http-server + sudo -u git -H git checkout 0.2.14 sudo -u git -H make ### Initialize Database and Activate Advanced Features diff --git a/doc/update/7.14-to-8.0.md b/doc/update/7.14-to-8.0.md index 7ad4935e839..305017b7048 100644 --- a/doc/update/7.14-to-8.0.md +++ b/doc/update/7.14-to-8.0.md @@ -84,6 +84,7 @@ Now we download `gitlab-git-http-server` and install it in `/home/git/gitlab-git cd /home/git sudo -u git -H git clone https://gitlab.com/gitlab-org/gitlab-git-http-server.git cd gitlab-git-http-server +sudo -u git -H git checkout 0.2.14 sudo -u git -H make ``` -- cgit v1.2.1 From 999051bc91f02292e28582ce538f0d147c7b665e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 15 Oct 2015 14:59:06 +0200 Subject: Use gitlab-git-http-server 0.3.0 --- GITLAB_GIT_HTTP_SERVER_VERSION | 2 +- doc/install/installation.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/GITLAB_GIT_HTTP_SERVER_VERSION b/GITLAB_GIT_HTTP_SERVER_VERSION index 769ed6ae790..0d91a54c7d4 100644 --- a/GITLAB_GIT_HTTP_SERVER_VERSION +++ b/GITLAB_GIT_HTTP_SERVER_VERSION @@ -1 +1 @@ -0.2.14 +0.3.0 diff --git a/doc/install/installation.md b/doc/install/installation.md index acc4e505971..e09890dc296 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -325,7 +325,7 @@ GitLab Shell is an SSH access and repository management software developed speci cd /home/git sudo -u git -H git clone https://gitlab.com/gitlab-org/gitlab-git-http-server.git cd gitlab-git-http-server - sudo -u git -H git checkout 0.2.14 + sudo -u git -H git checkout 0.3.0 sudo -u git -H make ### Initialize Database and Activate Advanced Features -- cgit v1.2.1 From 33574c02782824bb1c592a0f783b02dc0f7cca0a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 16 Oct 2015 09:31:35 +0200 Subject: Fix padding of outdated discussion item. --- CHANGELOG | 1 + app/assets/stylesheets/pages/notes.scss | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 39926692147..711d1fcba62 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -62,6 +62,7 @@ v 8.1.0 (unreleased) - Only render 404 page from /public - Hide passwords from services API (Alex Lossent) - Fix: Images cannot show when projects' path was changed + - Fix padding of outdated discussion item. v 8.0.4 - Fix Message-ID header to be RFC 2111-compliant to prevent e-mails being dropped (Stan Hu) diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index abb03b07f51..1980fe0d458 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -30,7 +30,6 @@ ul.notes { .discussion-header, .note-header { @extend .cgray; - padding-bottom: 15px; a:hover { text-decoration: none; @@ -75,6 +74,10 @@ ul.notes { } } + .discussion-body { + padding-top: 15px; + } + .discussion { overflow: hidden; display: block; -- cgit v1.2.1 From 9f9f0c35ecd9f7a5a057030253791d051f832f6d Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 12 Oct 2015 12:04:20 +0200 Subject: Show merge requests which close current issue --- CHANGELOG | 1 + app/assets/stylesheets/pages/issues.scss | 5 +++++ app/controllers/projects/issues_controller.rb | 7 +++++++ app/controllers/projects/merge_requests_controller.rb | 2 +- app/helpers/issues_helper.rb | 4 ++++ app/models/issue.rb | 10 ++++++++++ app/models/merge_request.rb | 4 ++++ app/views/projects/issues/_closed_by_box.html.haml | 6 ++++++ app/views/projects/issues/show.html.haml | 3 ++- 9 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 app/views/projects/issues/_closed_by_box.html.haml diff --git a/CHANGELOG b/CHANGELOG index 39926692147..e15fc9cb24a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.1.0 (unreleased) - Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu) - Speed up load times of issue detail pages by roughly 1.5x + - If a merge request is to close an issue, show this on the issue page (Zeger-Jan van de Weg) - Make diff file view easier to use on mobile screens (Stan Hu) - Improved performance of finding users by username or Email address - Fix bug where merge request comments created by API would not trigger notifications (Stan Hu) diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss index 4bf58cb4a59..41c069f0ad3 100644 --- a/app/assets/stylesheets/pages/issues.scss +++ b/app/assets/stylesheets/pages/issues.scss @@ -132,6 +132,11 @@ form.edit-issue { } } +.issue-closed-by-widget { + padding: 16px 0; + margin: 0px; +} + .issue-form .select2-container { width: 250px !important; } diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 97485c101fb..eaf14009242 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -14,6 +14,9 @@ class Projects::IssuesController < Projects::ApplicationController # Allow issues bulk update before_action :authorize_admin_issues!, only: [:bulk_update] + # Cross-reference merge requests + before_action :closed_by_merge_requests, only: [:show] + respond_to :html def index @@ -112,6 +115,10 @@ class Projects::IssuesController < Projects::ApplicationController render nothing: true end + def closed_by_merge_requests + @closed_by_mr = @issue.closed_by_merge_requests(current_user) + end + protected def issue diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 98df6984bf7..0d9c5461959 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -259,7 +259,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @commits = @merge_request.commits @merge_request_diff = @merge_request.merge_request_diff - + if @merge_request.locked_long_ago? @merge_request.unlock_mr @merge_request.close diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 6ddb37cd0dc..1adbf3a79b1 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -83,6 +83,10 @@ module IssuesHelper end end + def merge_requests_sentence(merge_requests) + merge_requests.map(&:to_reference).to_sentence + end + # Required for Gitlab::Markdown::IssueReferenceFilter module_function :url_for_issue end diff --git a/app/models/issue.rb b/app/models/issue.rb index fc7e9abe29e..c24a329847c 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -95,4 +95,14 @@ class Issue < ActiveRecord::Base def source_project project end + + # From all notes on this issue, we'll select the system notes about linked + # merge requests. Of those, the MRs closing `self` are returned. + def closed_by_merge_requests(current_user) + notes.system.flat_map do |note| + ext = Gitlab::ReferenceExtractor.new(self.project, current_user) + ext.analyze(note.note) + ext.merge_requests + end.uniq.select { |mr| mr.closes_issue?(self) } + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index c83b15c7d39..3ae74ceac68 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -294,6 +294,10 @@ class MergeRequest < ActiveRecord::Base target_project end + def closes_issue?(issue) + closes_issues.include?(issue) + end + # Return the set of issues that will be closed if this merge request is accepted. def closes_issues(current_user = self.author) if target_branch == project.default_branch diff --git a/app/views/projects/issues/_closed_by_box.html.haml b/app/views/projects/issues/_closed_by_box.html.haml new file mode 100644 index 00000000000..fe886b6d7d7 --- /dev/null +++ b/app/views/projects/issues/_closed_by_box.html.haml @@ -0,0 +1,6 @@ +.issue-closed-by-widget + %i.fa.fa-check + - if @closed_by_mr.count == 1 + This issue will be closed when #{gfm(@closed_by_mr.first.to_reference)} is accepted. + -else + This issue will be closed when any of merge requests #{gfm(merge_requests_sentence(@closed_by_mr.sort))} is accepted. diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index 5cb814c9ea8..309f276882d 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -46,6 +46,7 @@ = markdown(@issue.description) %textarea.hidden.js-task-list-field = @issue.description - + - if @closed_by_mr.present? + = render 'projects/issues/closed_by_box' .issue-discussion = render 'projects/issues/discussion' -- cgit v1.2.1 From 94a788f66dfcc13ad02855b05c38826f958038af Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 13 Oct 2015 09:41:46 +0200 Subject: Only accept open issues and merge requests --- app/controllers/projects/issues_controller.rb | 2 +- app/helpers/issues_helper.rb | 2 +- app/models/concerns/issuable.rb | 4 +++ app/models/issue.rb | 10 +++--- app/models/merge_request.rb | 4 --- app/views/projects/issues/_closed_by_box.html.haml | 9 ++---- app/views/projects/issues/show.html.haml | 2 +- spec/helpers/issues_helper_spec.rb | 10 ++++++ spec/models/concerns/issuable_spec.rb | 1 - spec/models/issue_spec.rb | 37 ++++++++++++++++++++++ 10 files changed, 62 insertions(+), 19 deletions(-) diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index eaf14009242..cc8321d97ad 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -116,7 +116,7 @@ class Projects::IssuesController < Projects::ApplicationController end def closed_by_merge_requests - @closed_by_mr = @issue.closed_by_merge_requests(current_user) + @closed_by_merge_requests ||= @issue.closed_by_merge_requests(current_user) end protected diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 1adbf3a79b1..fda18e7b316 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -84,7 +84,7 @@ module IssuesHelper end def merge_requests_sentence(merge_requests) - merge_requests.map(&:to_reference).to_sentence + merge_requests.map(&:to_reference).to_sentence(last_word_connector: ', or ') end # Required for Gitlab::Markdown::IssueReferenceFilter diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 0e8bcc1a4ec..efa6a269992 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -86,6 +86,10 @@ module Issuable assignee_id_changed? end + def open? + opened? || reopened? + end + # # Votes # diff --git a/app/models/issue.rb b/app/models/issue.rb index c24a329847c..72183108033 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -98,11 +98,11 @@ class Issue < ActiveRecord::Base # From all notes on this issue, we'll select the system notes about linked # merge requests. Of those, the MRs closing `self` are returned. - def closed_by_merge_requests(current_user) + def closed_by_merge_requests(current_user = nil) + return [] unless open? + notes.system.flat_map do |note| - ext = Gitlab::ReferenceExtractor.new(self.project, current_user) - ext.analyze(note.note) - ext.merge_requests - end.uniq.select { |mr| mr.closes_issue?(self) } + note.all_references(current_user).merge_requests + end.uniq.select { |mr| mr.open? && mr.closes_issue?(self) } end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 3ae74ceac68..0042b95c4f1 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -222,10 +222,6 @@ class MergeRequest < ActiveRecord::Base self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last end - def open? - opened? || reopened? - end - def work_in_progress? !!(title =~ /\A\[?WIP\]?:? /i) end diff --git a/app/views/projects/issues/_closed_by_box.html.haml b/app/views/projects/issues/_closed_by_box.html.haml index fe886b6d7d7..aef352029d0 100644 --- a/app/views/projects/issues/_closed_by_box.html.haml +++ b/app/views/projects/issues/_closed_by_box.html.haml @@ -1,6 +1,3 @@ -.issue-closed-by-widget - %i.fa.fa-check - - if @closed_by_mr.count == 1 - This issue will be closed when #{gfm(@closed_by_mr.first.to_reference)} is accepted. - -else - This issue will be closed when any of merge requests #{gfm(merge_requests_sentence(@closed_by_mr.sort))} is accepted. +.issue-closed-by-widget + = icon('check') + This issue will be closed automatically when merge request #{gfm(merge_requests_sentence(@closed_by_merge_requests.sort))} is accepted. diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index 309f276882d..f01bf2505da 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -46,7 +46,7 @@ = markdown(@issue.description) %textarea.hidden.js-task-list-field = @issue.description - - if @closed_by_mr.present? + - if @closed_by_merge_requests.present? = render 'projects/issues/closed_by_box' .issue-discussion = render 'projects/issues/discussion' diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb index c08ddb4cae1..78a6b631eb2 100644 --- a/spec/helpers/issues_helper_spec.rb +++ b/spec/helpers/issues_helper_spec.rb @@ -117,4 +117,14 @@ describe IssuesHelper do end end + describe "#merge_requests_sentence" do + subject { merge_requests_sentence(merge_requests)} + let(:merge_requests) do + [ build(:merge_request, iid: 1), build(:merge_request, iid: 2), + build(:merge_request, iid: 3)] + end + + it { is_expected.to eq("!1, !2, or !3") } + end + end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 8f706f8934b..0f13c4410cd 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -68,7 +68,6 @@ describe Issue, "Issuable" do end end - describe "#to_hook_data" do let(:hook_data) { issue.to_hook_data(user) } diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 623332cd2f9..c9aa1b063c6 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -68,6 +68,43 @@ describe Issue do end end + describe '#closed_by_merge_requests' do + let(:project) { create(:project) } + let(:issue) { create(:issue, project: project, state: "opened")} + let(:closed_issue) { build(:issue, project: project, state: "closed")} + + let(:mr) do + opts = { + title: 'Awesome merge_request', + description: "Fixes #{issue.to_reference}", + source_branch: 'feature', + target_branch: 'master' + } + MergeRequests::CreateService.new(project, project.owner, opts).execute + end + + let(:closed_mr) do + opts = { + title: 'Awesome merge_request 2', + description: "Fixes #{issue.to_reference}", + source_branch: 'feature', + target_branch: 'master', + state: 'closed' + } + MergeRequests::CreateService.new(project, project.owner, opts).execute + end + + it 'returns the merge request to close this issue' do + allow(mr).to receive(:closes_issue?).with(issue).and_return(true) + + expect(issue.closed_by_merge_requests).to eq([mr]) + end + + it "returns an empty array when the current issue is closed already" do + expect(closed_issue.closed_by_merge_requests).to eq([]) + end + end + it_behaves_like 'an editable mentionable' do subject { create(:issue) } -- cgit v1.2.1 From 64504b636470ad1048ba6310d6bd2dff8a28b914 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 13 Oct 2015 16:04:36 +0200 Subject: Add an index to milestones title and label title --- db/migrate/20151013133938_add_index_to_milestones.rb | 6 ++++++ db/schema.rb | 2 ++ 2 files changed, 8 insertions(+) create mode 100644 db/migrate/20151013133938_add_index_to_milestones.rb diff --git a/db/migrate/20151013133938_add_index_to_milestones.rb b/db/migrate/20151013133938_add_index_to_milestones.rb new file mode 100644 index 00000000000..41cd91e570b --- /dev/null +++ b/db/migrate/20151013133938_add_index_to_milestones.rb @@ -0,0 +1,6 @@ +class AddIndexToMilestones < ActiveRecord::Migration + def change + add_index :milestones, :title + add_index :labels, :title + end +end diff --git a/db/schema.rb b/db/schema.rb index 7a11dfca034..68bd9d2c3e5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -410,6 +410,7 @@ ActiveRecord::Schema.define(version: 20151008130321) do end add_index "labels", ["project_id"], name: "index_labels_on_project_id", using: :btree + add_index "labels", ["title"], name: "index_labels_on_title", using: :btree create_table "members", force: true do |t| t.integer "access_level", null: false @@ -491,6 +492,7 @@ ActiveRecord::Schema.define(version: 20151008130321) do add_index "milestones", ["due_date"], name: "index_milestones_on_due_date", using: :btree add_index "milestones", ["project_id", "iid"], name: "index_milestones_on_project_id_and_iid", unique: true, using: :btree add_index "milestones", ["project_id"], name: "index_milestones_on_project_id", using: :btree + add_index "milestones", ["title"], name: "index_milestones_on_title", using: :btree create_table "namespaces", force: true do |t| t.string "name", null: false -- cgit v1.2.1 From b5762104abbf373d69a20532de08564eb9ae93f6 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 13 Oct 2015 16:05:04 +0200 Subject: Minor refactoring in seeding --- db/fixtures/development/05_users.rb | 4 ++-- db/fixtures/development/07_milestones.rb | 2 +- db/fixtures/development/09_issues.rb | 2 +- db/fixtures/development/12_snippets.rb | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/db/fixtures/development/05_users.rb b/db/fixtures/development/05_users.rb index 378354efd5a..03da29c4c68 100644 --- a/db/fixtures/development/05_users.rb +++ b/db/fixtures/development/05_users.rb @@ -1,5 +1,5 @@ Gitlab::Seeder.quiet do - (2..20).each do |i| + 20.times do |i| begin User.create!( username: FFaker::Internet.user_name, @@ -15,7 +15,7 @@ Gitlab::Seeder.quiet do end end - (1..5).each do |i| + 5.times do |i| begin User.create!( username: "user#{i}", diff --git a/db/fixtures/development/07_milestones.rb b/db/fixtures/development/07_milestones.rb index a43116829d9..e028ac82ba3 100644 --- a/db/fixtures/development/07_milestones.rb +++ b/db/fixtures/development/07_milestones.rb @@ -1,6 +1,6 @@ Gitlab::Seeder.quiet do Project.all.each do |project| - (1..5).each do |i| + 5.times do |i| milestone_params = { title: "v#{i}.0", description: FFaker::Lorem.sentence, diff --git a/db/fixtures/development/09_issues.rb b/db/fixtures/development/09_issues.rb index c636e96381c..4fa572fca9b 100644 --- a/db/fixtures/development/09_issues.rb +++ b/db/fixtures/development/09_issues.rb @@ -1,6 +1,6 @@ Gitlab::Seeder.quiet do Project.all.each do |project| - (1..10).each do |i| + 10.times do issue_params = { title: FFaker::Lorem.sentence(6), description: FFaker::Lorem.sentence, diff --git a/db/fixtures/development/12_snippets.rb b/db/fixtures/development/12_snippets.rb index 3bd4b442ade..74898544a69 100644 --- a/db/fixtures/development/12_snippets.rb +++ b/db/fixtures/development/12_snippets.rb @@ -22,7 +22,7 @@ class Member < ActiveRecord::Base end eos - (1..50).each do |i| + 50.times do |i| user = User.all.sample PersonalSnippet.seed(:id, [{ -- cgit v1.2.1 From ac44e3844deac1c13f35ca0e9a7ce995be58aab7 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 14 Oct 2015 12:20:48 +0200 Subject: Add project scope to milestone search --- app/finders/issuable_finder.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 97c7e74c294..d60f36e1aff 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -81,7 +81,14 @@ class IssuableFinder @milestones = if milestones? - Milestone.where(title: params[:milestone_title]) + scope = + if project + project.milestones + else + Milestone.all + end + + scope.where(title: params[:milestone_title]) else nil end -- cgit v1.2.1 From 9127ae5ca80aa06b0a83d275e2a2d9b7ccfbfc3d Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 14 Oct 2015 12:23:49 +0200 Subject: Improve performance of queries Credits to Douwe Maan --- CHANGELOG | 1 + app/finders/issuable_finder.rb | 74 ++++++++++++++-------- .../20151013133938_add_index_to_milestones.rb | 6 -- 3 files changed, 47 insertions(+), 34 deletions(-) delete mode 100644 db/migrate/20151013133938_add_index_to_milestones.rb diff --git a/CHANGELOG b/CHANGELOG index 814a6772cfd..8f696009d61 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -63,6 +63,7 @@ v 8.1.0 (unreleased) - Only render 404 page from /public - Hide passwords from services API (Alex Lossent) - Fix: Images cannot show when projects' path was changed + - Optimize query when filtering on issuables (Zeger-Jan van de Weg) v 8.0.4 - Fix Message-ID header to be RFC 2111-compliant to prevent e-mails being dropped (Stan Hu) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index d60f36e1aff..3170c0f672e 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -53,15 +53,36 @@ class IssuableFinder end end + def project? + params[:project_id].present? + end + def project return @project if defined?(@project) - @project = - if params[:project_id].present? - Project.find(params[:project_id]) - else - nil - end + if project? + @project = Project.find(params[:project_id]) + + unless Ability.abilities.allowed?(current_user, :read_project, @project) + @project = nil + end + else + @project = nil + end + + @project + end + + def projects + return if project? + + return @projects if defined?(@projects) + + if current_user && params[:authorized_only].presence && !current_user_related? + current_user.authorized_projects + else + ProjectsFinder.new.execute(current_user) + end end def search @@ -84,8 +105,10 @@ class IssuableFinder scope = if project project.milestones + elsif projects + Milestone.where(project_id: projects) else - Milestone.all + Milestone.none end scope.where(title: params[:milestone_title]) @@ -127,19 +150,7 @@ class IssuableFinder private def init_collection - table_name = klass.table_name - - if project - if Ability.abilities.allowed?(current_user, :read_project, project) - project.send(table_name) - else - [] - end - elsif current_user && params[:authorized_only].presence && !current_user_related? - klass.of_projects(current_user.authorized_projects).references(:project) - else - klass.of_projects(ProjectsFinder.new.execute(current_user)).references(:project) - end + klass.all end def by_scope(items) @@ -177,7 +188,14 @@ class IssuableFinder end def by_project(items) - items = items.of_projects(project.id) if project + items = + if project + items.of_projects(project) + elsif projects + items.of_projects(projects).references(:project) + else + items.none + end items end @@ -223,17 +241,17 @@ class IssuableFinder def by_label(items) if params[:label_name].present? if params[:label_name] == Label::None.title - item_ids = LabelLink.where(target_type: klass.name).pluck(:target_id) - - items = items.where('id NOT IN (?)', item_ids) + items = items. + joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{klass.name}' AND label_links.target_id = #{klass.table_name}.id"). + where(label_links: { id: nil }) else label_names = params[:label_name].split(",") - item_ids = LabelLink.joins(:label). - where('labels.title in (?)', label_names). - where(target_type: klass.name).pluck(:target_id) + items = items.joins(:labels).where(labels: { title: label_names }) - items = items.where(id: item_ids) + if project + items = items.where('labels.project_id = :id', id: project.id) + end end end diff --git a/db/migrate/20151013133938_add_index_to_milestones.rb b/db/migrate/20151013133938_add_index_to_milestones.rb deleted file mode 100644 index 41cd91e570b..00000000000 --- a/db/migrate/20151013133938_add_index_to_milestones.rb +++ /dev/null @@ -1,6 +0,0 @@ -class AddIndexToMilestones < ActiveRecord::Migration - def change - add_index :milestones, :title - add_index :labels, :title - end -end -- cgit v1.2.1 From 0108cdf49514dcaccc6a53c7b6e257fa9acfea98 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 16 Oct 2015 11:43:26 +0200 Subject: Improve performance of filtering issues by milestone --- app/finders/issuable_finder.rb | 63 ++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 3170c0f672e..f00bb02d0fb 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -74,11 +74,11 @@ class IssuableFinder end def projects - return if project? - return @projects if defined?(@projects) - if current_user && params[:authorized_only].presence && !current_user_related? + if project? + project + elsif current_user && params[:authorized_only].presence && !current_user_related? current_user.authorized_projects else ProjectsFinder.new.execute(current_user) @@ -102,14 +102,7 @@ class IssuableFinder @milestones = if milestones? - scope = - if project - project.milestones - elsif projects - Milestone.where(project_id: projects) - else - Milestone.none - end + scope = Milestone.where(project_id: projects) scope.where(title: params[:milestone_title]) else @@ -117,6 +110,14 @@ class IssuableFinder end end + def labels? + params[:label_name].present? + end + + def no_labels? + labels? && params[:label_name] == Label::None.title + end + def assignee? params[:assignee_id].present? end @@ -189,9 +190,7 @@ class IssuableFinder def by_project(items) items = - if project - items.of_projects(project) - elsif projects + if projects items.of_projects(projects).references(:project) else items.none @@ -210,18 +209,6 @@ class IssuableFinder items.sort(params[:sort]) end - def by_milestone(items) - if milestones? - if no_milestones? - items = items.where(milestone_id: [-1, nil]) - else - items = items.where(milestone_id: milestones.try(:pluck, :id)) - end - end - - items - end - def by_assignee(items) if assignee? items = items.where(assignee_id: assignee.try(:id)) @@ -238,9 +225,25 @@ class IssuableFinder items end + def by_milestone(items) + if milestones? + if no_milestones? + items = items.where(milestone_id: [-1, nil]) + else + items = items.joins(:milestone).where(milestones: { title: params[:milestone_title] }) + + if projects + items = items.where(milestones: { project_id: projects }) + end + end + end + + items + end + def by_label(items) - if params[:label_name].present? - if params[:label_name] == Label::None.title + if labels? + if no_labels? items = items. joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{klass.name}' AND label_links.target_id = #{klass.table_name}.id"). where(label_links: { id: nil }) @@ -249,8 +252,8 @@ class IssuableFinder items = items.joins(:labels).where(labels: { title: label_names }) - if project - items = items.where('labels.project_id = :id', id: project.id) + if projects + items = items.where(labels: { project_id: projects }) end end end -- cgit v1.2.1 From 7c85ebf6dc9c083d53642cbe2b6a5276c6b95aa6 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 16 Oct 2015 13:24:28 +0200 Subject: Partly implement new UI for user page Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/framework/blocks.scss | 45 ++++++++++++++ app/assets/stylesheets/pages/profile.scss | 6 ++ app/views/admin/users/_profile.html.haml | 31 ++++++++++ app/views/admin/users/show.html.haml | 2 +- app/views/users/_profile.html.haml | 31 ---------- app/views/users/calendar.html.haml | 6 +- app/views/users/show.html.haml | 90 ++++++++++++++++++---------- features/steps/abuse_reports.rb | 2 +- 8 files changed, 143 insertions(+), 70 deletions(-) create mode 100644 app/views/admin/users/_profile.html.haml delete mode 100644 app/views/users/_profile.html.haml diff --git a/app/assets/stylesheets/framework/blocks.scss b/app/assets/stylesheets/framework/blocks.scss index 6ce34b5c3e8..a09339050c5 100644 --- a/app/assets/stylesheets/framework/blocks.scss +++ b/app/assets/stylesheets/framework/blocks.scss @@ -60,3 +60,48 @@ line-height: 42px; } } + +.cover-block { + text-align: center; + background: #f7f8fa; + margin: -$gl-padding; + margin-bottom: 0; + padding: 44px $gl-padding; + border-bottom: 1px solid $border-color; + position: relative; + + .avatar-holder { + margin-bottom: 16px; + + .avatar, .identicon { + margin: 0 auto; + float: none; + } + + .identicon { + @include border-radius(50%); + } + } + + .cover-title { + color: $gl-header-color; + margin: 0; + font-size: 23px; + font-weight: normal; + margin: 16px 0 5px 0; + color: #4c4e54; + font-size: 23px; + line-height: 1.1; + } + + .cover-desc { + padding: 0 $gl-padding; + color: $gl-text-color; + } + + .cover-controls { + position: absolute; + top: 10px; + right: 10px; + } +} diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss index 8e4f0eb2b25..b7391e5303b 100644 --- a/app/assets/stylesheets/pages/profile.scss +++ b/app/assets/stylesheets/pages/profile.scss @@ -47,3 +47,9 @@ } } } + +.calendar-hint { + margin-top: -12px; + float: right; + font-size: 12px; +} diff --git a/app/views/admin/users/_profile.html.haml b/app/views/admin/users/_profile.html.haml new file mode 100644 index 00000000000..90d9980c85c --- /dev/null +++ b/app/views/admin/users/_profile.html.haml @@ -0,0 +1,31 @@ +.panel.panel-default + .panel-heading + Profile + %ul.well-list + %li + %span.light Member since + %strong= user.created_at.stamp("Aug 21, 2011") + - unless user.public_email.blank? + %li + %span.light E-mail: + %strong= link_to user.public_email, "mailto:#{user.public_email}" + - unless user.skype.blank? + %li + %span.light Skype: + %strong= link_to user.skype, "skype:#{user.skype}" + - unless user.linkedin.blank? + %li + %span.light LinkedIn: + %strong= link_to user.linkedin, "http://www.linkedin.com/in/#{user.linkedin}" + - unless user.twitter.blank? + %li + %span.light Twitter: + %strong= link_to user.twitter, "http://www.twitter.com/#{user.twitter}" + - unless user.website_url.blank? + %li + %span.light Website: + %strong= link_to user.short_website_url, user.full_website_url + - unless user.location.blank? + %li + %span.light Location: + %strong= user.location diff --git a/app/views/admin/users/show.html.haml b/app/views/admin/users/show.html.haml index 231bcb0426f..0848504b7a6 100644 --- a/app/views/admin/users/show.html.haml +++ b/app/views/admin/users/show.html.haml @@ -14,7 +14,7 @@ %strong = link_to user_path(@user) do = @user.username - = render 'users/profile', user: @user + = render 'admin/users/profile', user: @user .panel.panel-default .panel-heading diff --git a/app/views/users/_profile.html.haml b/app/views/users/_profile.html.haml deleted file mode 100644 index 90d9980c85c..00000000000 --- a/app/views/users/_profile.html.haml +++ /dev/null @@ -1,31 +0,0 @@ -.panel.panel-default - .panel-heading - Profile - %ul.well-list - %li - %span.light Member since - %strong= user.created_at.stamp("Aug 21, 2011") - - unless user.public_email.blank? - %li - %span.light E-mail: - %strong= link_to user.public_email, "mailto:#{user.public_email}" - - unless user.skype.blank? - %li - %span.light Skype: - %strong= link_to user.skype, "skype:#{user.skype}" - - unless user.linkedin.blank? - %li - %span.light LinkedIn: - %strong= link_to user.linkedin, "http://www.linkedin.com/in/#{user.linkedin}" - - unless user.twitter.blank? - %li - %span.light Twitter: - %strong= link_to user.twitter, "http://www.twitter.com/#{user.twitter}" - - unless user.website_url.blank? - %li - %span.light Website: - %strong= link_to user.short_website_url, user.full_website_url - - unless user.location.blank? - %li - %span.light Location: - %strong= user.location diff --git a/app/views/users/calendar.html.haml b/app/views/users/calendar.html.haml index 922b0c6cebf..7f29918dba3 100644 --- a/app/views/users/calendar.html.haml +++ b/app/views/users/calendar.html.haml @@ -1,7 +1,3 @@ -%h4 - Contributions calendar - .pull-right - %small Issues, merge requests and push events #cal-heatmap.calendar :javascript new Calendar( @@ -10,3 +6,5 @@ #{@starting_month}, '#{user_calendar_activities_path}' ); + +.calendar-hint Summary of issues, merge requests and push events diff --git a/app/views/users/show.html.haml b/app/views/users/show.html.haml index 2a64708d07c..4ea4a1f92c2 100644 --- a/app/views/users/show.html.haml +++ b/app/views/users/show.html.haml @@ -6,47 +6,72 @@ = render 'shared/show_aside' -.row - %section.col-md-7 - .header-with-avatar - = link_to avatar_icon(@user, 400), target: '_blank' do - = image_tag avatar_icon(@user, 90), class: "avatar avatar-tile s90", alt: '' - %h3 - = @user.name - - if @user == current_user - .pull-right.hidden-xs - = link_to profile_path, class: 'btn btn-sm' do - = icon('user') - Profile settings - - elsif current_user - .report_abuse.pull-right - - if @user.abuse_report - %span#report_abuse_btn.light.btn.btn-sm.btn-close{title: 'Already reported for abuse', data: {toggle: 'tooltip', placement: 'right', container: 'body'}} - = icon('exclamation-circle') - - else - %a.light.btn.btn-sm{href: new_abuse_report_path(user_id: @user.id), title: 'Report abuse', data: {toggle: 'tooltip', placement: 'right', container: 'body'}} - = icon('exclamation-circle') +.cover-block + .avatar-holder + = link_to avatar_icon(@user, 400), target: '_blank' do + = image_tag avatar_icon(@user, 90), class: "avatar s90", alt: '' + .cover-title + = @user.name + + .cover-desc + %span + @#{@user.username}. + - if @user.bio.present? + %span + #{@user.bio}. + %span + Member since #{@user.created_at.stamp("Aug 21, 2011")} + + .cover-desc + - unless @user.public_email.blank? + = link_to @user.public_email, "mailto:#{@user.public_email}" + - unless @user.skype.blank? + · + = link_to "Skype", "skype:#{@user.skype}" + - unless @user.linkedin.blank? + · + = link_to "LinkedIn", "http://www.linkedin.com/in/#{@user.linkedin}" + - unless @user.twitter.blank? + · + = link_to "Twitter", "http://www.twitter.com/#{@user.twitter}" + - unless @user.website_url.blank? + · + = link_to @user.short_website_url, @user.full_website_url + - unless @user.location.blank? + · + = @user.location - .username - @#{@user.username} - .description - - if @user.bio.present? - = @user.bio - .clearfix + .cover-controls + - if @user == current_user + = link_to profile_path, class: 'btn btn-gray' do + = icon('pencil') + - elsif current_user + .report-abuse + - if @user.abuse_report + %button.btn.btn-danger{ title: 'Already reported for abuse', + data: { toggle: 'tooltip', placement: 'left', container: 'body' }} + = icon('exclamation-circle') + - else + = link_to new_abuse_report_path(user_id: @user.id), class: 'btn btn-gray', + title: 'Report abuse', data: {toggle: 'tooltip', placement: 'left', container: 'body'} do + = icon('exclamation-circle') +.gray-content-block.second-block + .user-calendar + %h4.center.light + %i.fa.fa-spinner.fa-spin + .user-calendar-activities + + +.row.prepend-top-20 + %section.col-md-7 - if @groups.any? .prepend-top-20 %h4 Groups = render 'groups', groups: @groups %hr - .hidden-xs - .user-calendar - %h4.center.light - %i.fa.fa-spinner.fa-spin - .user-calendar-activities - %hr %h4 User Activity @@ -59,7 +84,6 @@ .content_list = spinner %aside.col-md-5 - = render 'profile', user: @user = render 'projects', projects: @projects, contributed_projects: @contributed_projects :coffeescript diff --git a/features/steps/abuse_reports.rb b/features/steps/abuse_reports.rb index 56652ff6f05..499accb0b08 100644 --- a/features/steps/abuse_reports.rb +++ b/features/steps/abuse_reports.rb @@ -23,7 +23,7 @@ class Spinach::Features::AbuseReports < Spinach::FeatureSteps end step 'I should see a red "Report abuse" button' do - expect(find(:css, '.report_abuse')).to have_selector(:css, 'span.btn-close') + expect(page).to have_button("Already reported for abuse") end def user_mike -- cgit v1.2.1 From a7fafee52124df6ffa9e6e1ecee6ff6884c6ce95 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Fri, 16 Oct 2015 08:45:50 -0500 Subject: Fix import from SVN link --- app/views/projects/imports/new.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/imports/new.html.haml b/app/views/projects/imports/new.html.haml index f8f2e192e29..92a87690c54 100644 --- a/app/views/projects/imports/new.html.haml +++ b/app/views/projects/imports/new.html.haml @@ -17,6 +17,6 @@ This URL must be publicly accessible or you can add a username and password like this: https://username:password@gitlab.com/company/project.git. %br The import will time out after 4 minutes. For big repositories, use a clone/push combination. - For SVN repositories, check #{link_to "this migrating from SVN doc.", "http://doc.gitlab.com/ce/workflow/migrating_from_svn.html"} + For SVN repositories, check #{link_to "this migrating from SVN doc.", "http://doc.gitlab.com/ce/workflow/importing/migrating_from_svn.html"} .form-actions = f.submit 'Start import', class: "btn btn-create", tabindex: 4 -- cgit v1.2.1 From 6909b309935f32da6bcccb1148242f19eec5de26 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 16 Oct 2015 16:39:31 +0200 Subject: Fix missing commit status for widget when no CI service is enabled --- .../merge_requests/widget/_heading.html.haml | 73 +++++++++++----------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 68dda1424cf..10efb811939 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -1,44 +1,43 @@ -- if @merge_request.has_ci? - - ci_commit = @merge_request.source_project.ci_commit(@merge_request.source_sha) - - if ci_commit - - status = ci_commit.status - .mr-widget-heading - .ci_widget{class: "ci-#{status}"} - = ci_status_icon(ci_commit) +- ci_commit = @merge_request.source_project.ci_commit(@merge_request.source_sha) +- if ci_commit + - status = ci_commit.status + .mr-widget-heading + .ci_widget{class: "ci-#{status}"} + = ci_status_icon(ci_commit) + %span CI build #{status} + for #{@merge_request.last_commit_short_sha}. + %span.ci-coverage + = link_to "View build details", ci_status_path(ci_commit) + +- elsif @merge_request.has_ci? + - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX + - # Remove in later versions when services like Jenkins will set CI status via Commit status API + .mr-widget-heading + - [:success, :skipped, :canceled, :failed, :running, :pending].each do |status| + .ci_widget{class: "ci-#{status}", style: "display:none"} + - if status == :success + - status = "passed" + = icon("check-circle") + - else + = icon("circle") %span CI build #{status} for #{@merge_request.last_commit_short_sha}. %span.ci-coverage - = link_to "View build details", ci_status_path(ci_commit) - - - else - - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX - - # Remove in later versions when services like Jenkins will set CI status via Commit status API - .mr-widget-heading - - [:success, :skipped, :canceled, :failed, :running, :pending].each do |status| - .ci_widget{class: "ci-#{status}", style: "display:none"} - - if status == :success - - status = "passed" - = icon("check-circle") - - else - = icon("circle") - %span CI build #{status} - for #{@merge_request.last_commit_short_sha}. - %span.ci-coverage - - if ci_build_details_path(@merge_request) - = link_to "View build details", ci_build_details_path(@merge_request), :"data-no-turbolink" => "data-no-turbolink" + - if ci_build_details_path(@merge_request) + = link_to "View build details", ci_build_details_path(@merge_request), :"data-no-turbolink" => "data-no-turbolink" - .ci_widget - = icon("spinner spin") - Checking CI status for #{@merge_request.last_commit_short_sha}… + .ci_widget + = icon("spinner spin") + Checking CI status for #{@merge_request.last_commit_short_sha}… - .ci_widget.ci-not_found{style: "display:none"} - = icon("times-circle") - Could not find CI status for #{@merge_request.last_commit_short_sha}. + .ci_widget.ci-not_found{style: "display:none"} + = icon("times-circle") + Could not find CI status for #{@merge_request.last_commit_short_sha}. - .ci_widget.ci-error{style: "display:none"} - = icon("times-circle") - Could not connect to the CI server. Please check your settings and try again. + .ci_widget.ci-error{style: "display:none"} + = icon("times-circle") + Could not connect to the CI server. Please check your settings and try again. - :coffeescript - $ -> - merge_request_widget.getCiStatus() + :coffeescript + $ -> + merge_request_widget.getCiStatus() -- cgit v1.2.1 From f405954764b8bea32e54b3cf59e06a5469781bf3 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 16 Oct 2015 21:58:30 +0200 Subject: Added indexes for notes.line_code and CI columns This adds indexes for the following columns: * notes.line_code * ci_projects.gitlab_id * ci_projects.shared_runners_enabled * ci_builds.type * ci_builds.status --- db/migrate/20151016195451_add_ci_builds_and_projects_indexes.rb | 9 +++++++++ db/migrate/20151016195706_add_notes_line_code_index.rb | 5 +++++ db/schema.rb | 8 +++++++- 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20151016195451_add_ci_builds_and_projects_indexes.rb create mode 100644 db/migrate/20151016195706_add_notes_line_code_index.rb diff --git a/db/migrate/20151016195451_add_ci_builds_and_projects_indexes.rb b/db/migrate/20151016195451_add_ci_builds_and_projects_indexes.rb new file mode 100644 index 00000000000..7f1af1c7583 --- /dev/null +++ b/db/migrate/20151016195451_add_ci_builds_and_projects_indexes.rb @@ -0,0 +1,9 @@ +class AddCiBuildsAndProjectsIndexes < ActiveRecord::Migration + def change + add_index :ci_projects, :gitlab_id + add_index :ci_projects, :shared_runners_enabled + + add_index :ci_builds, :type + add_index :ci_builds, :status + end +end diff --git a/db/migrate/20151016195706_add_notes_line_code_index.rb b/db/migrate/20151016195706_add_notes_line_code_index.rb new file mode 100644 index 00000000000..aeeb1a759fa --- /dev/null +++ b/db/migrate/20151016195706_add_notes_line_code_index.rb @@ -0,0 +1,5 @@ +class AddNotesLineCodeIndex < ActiveRecord::Migration + def change + add_index :notes, :line_code + end +end diff --git a/db/schema.rb b/db/schema.rb index 885756fef12..886b05f3e56 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20151016131433) do +ActiveRecord::Schema.define(version: 20151016195706) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -115,6 +115,8 @@ ActiveRecord::Schema.define(version: 20151016131433) do add_index "ci_builds", ["project_id", "commit_id"], name: "index_ci_builds_on_project_id_and_commit_id", using: :btree add_index "ci_builds", ["project_id"], name: "index_ci_builds_on_project_id", using: :btree add_index "ci_builds", ["runner_id"], name: "index_ci_builds_on_runner_id", using: :btree + add_index "ci_builds", ["status"], name: "index_ci_builds_on_status", using: :btree + add_index "ci_builds", ["type"], name: "index_ci_builds_on_type", using: :btree create_table "ci_commits", force: true do |t| t.integer "project_id" @@ -190,6 +192,9 @@ ActiveRecord::Schema.define(version: 20151016131433) do t.text "generated_yaml_config" end + add_index "ci_projects", ["gitlab_id"], name: "index_ci_projects_on_gitlab_id", using: :btree + add_index "ci_projects", ["shared_runners_enabled"], name: "index_ci_projects_on_shared_runners_enabled", using: :btree + create_table "ci_runner_projects", force: true do |t| t.integer "runner_id", null: false t.integer "project_id", null: false @@ -530,6 +535,7 @@ ActiveRecord::Schema.define(version: 20151016131433) do add_index "notes", ["commit_id"], name: "index_notes_on_commit_id", using: :btree add_index "notes", ["created_at", "id"], name: "index_notes_on_created_at_and_id", using: :btree add_index "notes", ["created_at"], name: "index_notes_on_created_at", using: :btree + add_index "notes", ["line_code"], name: "index_notes_on_line_code", using: :btree add_index "notes", ["noteable_id", "noteable_type"], name: "index_notes_on_noteable_id_and_noteable_type", using: :btree add_index "notes", ["noteable_type"], name: "index_notes_on_noteable_type", using: :btree add_index "notes", ["project_id", "noteable_type"], name: "index_notes_on_project_id_and_noteable_type", using: :btree -- cgit v1.2.1 From efd5472c4b957fd30e745914337f5c16be6ec8ea Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Sat, 17 Oct 2015 09:38:29 +0200 Subject: Hide Builds tab is GitLab CI is not enabled Signed-off-by: Dmitriy Zaporozhets --- app/helpers/projects_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index dbadbb74549..dd5e3828da2 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -113,7 +113,7 @@ module ProjectsHelper nav_tabs << :merge_requests end - if can?(current_user, :read_build, project) + if project.gitlab_ci? && can?(current_user, :read_build, project) nav_tabs << :builds end -- cgit v1.2.1 From ca3ce5c26c60de421e010491cf9166f2090c8cc8 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 17 Oct 2015 00:55:33 -0700 Subject: Fix nonatomic database update potentially causing project star counts to go negative The counter_cache decrement function is called when a project star is deleted, but there was no guarantee multiple workers would not attempt to delete the same item simultaneously. Use an atomic update to prevent the count from going negative. Closes #3067 --- CHANGELOG | 1 + app/models/user.rb | 15 +++++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a1161a7a527..2c8df909441 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -5,6 +5,7 @@ v 8.2.0 (unreleased) - Highlight comment based on anchor in URL v 8.1.0 (unreleased) + - Fix nonatomic database update potentially causing project star counts to go negative (Stan Hu) - Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu) - Speed up load times of issue detail pages by roughly 1.5x - Add a system note and update relevant merge requests when a branch is deleted or re-added (Stan Hu) diff --git a/app/models/user.rb b/app/models/user.rb index 17ccb3b8788..3b346c55edb 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -706,12 +706,15 @@ class User < ActiveRecord::Base end def toggle_star(project) - user_star_project = users_star_projects. - where(project: project, user: self).take - if user_star_project - user_star_project.destroy - else - UsersStarProject.create!(project: project, user: self) + UsersStarProject.transaction do + user_star_project = users_star_projects. + where(project: project, user: self).lock(true).first + + if user_star_project + user_star_project.destroy + else + UsersStarProject.create!(project: project, user: self) + end end end -- cgit v1.2.1 From 748631b5a3f350fb7dc51f3ed306d27c1c3bba92 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Sat, 17 Oct 2015 10:21:01 +0200 Subject: Redirect old CI project route to GitLab project Signed-off-by: Dmitriy Zaporozhets --- app/controllers/ci/projects_controller.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/controllers/ci/projects_controller.rb b/app/controllers/ci/projects_controller.rb index 7777aa18031..96649ab815a 100644 --- a/app/controllers/ci/projects_controller.rb +++ b/app/controllers/ci/projects_controller.rb @@ -7,6 +7,11 @@ module Ci before_action :no_cache, only: [:badge] protect_from_forgery + def show + # Temporary compatibility with CI badges pointing to CI project page + redirect_to namespace_project_path(project.gl_project.namespace, project.gl_project) + end + # Project status badge # Image with build status for sha or ref def badge -- cgit v1.2.1 From e9be3ec8ee8fe83e1e31a832e25223e5f2603074 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Sat, 17 Oct 2015 11:05:57 +0200 Subject: Temporary bring /ci page page with help information Signed-off-by: Dmitriy Zaporozhets --- app/controllers/ci/projects_controller.rb | 6 +++--- app/views/ci/projects/index.html.haml | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) create mode 100644 app/views/ci/projects/index.html.haml diff --git a/app/controllers/ci/projects_controller.rb b/app/controllers/ci/projects_controller.rb index 7777aa18031..b7c67b79890 100644 --- a/app/controllers/ci/projects_controller.rb +++ b/app/controllers/ci/projects_controller.rb @@ -1,8 +1,8 @@ module Ci class ProjectsController < Ci::ApplicationController - before_action :project - before_action :authenticate_user!, except: [:build, :badge] - before_action :authorize_access_project!, except: [:badge] + before_action :project, except: [:index] + before_action :authenticate_user!, except: [:index, :build, :badge] + before_action :authorize_access_project!, except: [:index, :badge] before_action :authorize_manage_project!, only: [:toggle_shared_runners, :dumped_yaml] before_action :no_cache, only: [:badge] protect_from_forgery diff --git a/app/views/ci/projects/index.html.haml b/app/views/ci/projects/index.html.haml new file mode 100644 index 00000000000..9c2290bc4a5 --- /dev/null +++ b/app/views/ci/projects/index.html.haml @@ -0,0 +1,20 @@ +.wiki + %h1 + GitLab CI is now integrated in GitLab UI + %h2 For existing projects + + %p + Check the following pages to find the CI status you're looking for: + + %ul + %li Projects page - shows CI status for each project. + %li Project commits page - show CI status for each commit. + + + + %h2 For new projects + + %p + If you want to enable CI for a new project it is easy as adding + = link_to ".gitlab-ci.yml", "http://doc.gitlab.com/ce/ci/yaml/README.html" + file to your repository -- cgit v1.2.1 From cfa3602a1303c39e78d5bf23ad0ab34b1966d397 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:15:32 +0200 Subject: Move last push info at top of project page. --- app/views/projects/_activity.html.haml | 1 - app/views/projects/activity.html.haml | 2 ++ app/views/projects/show.html.haml | 3 +-- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/projects/_activity.html.haml b/app/views/projects/_activity.html.haml index c2683bc6219..012858f70b4 100644 --- a/app/views/projects/_activity.html.haml +++ b/app/views/projects/_activity.html.haml @@ -1,4 +1,3 @@ -= render 'projects/last_push' .gray-content-block.activity-filter-block - if current_user .pull-right diff --git a/app/views/projects/activity.html.haml b/app/views/projects/activity.html.haml index 555ed76426d..69fa4ad37c4 100644 --- a/app/views/projects/activity.html.haml +++ b/app/views/projects/activity.html.haml @@ -1,4 +1,6 @@ - page_title "Activity" - header_title project_title(@project, "Activity", activity_project_path(@project)) += render 'projects/last_push' + = render 'projects/activity' diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index fdd3d0b322e..f8c06d8b06b 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -7,8 +7,7 @@ = render 'shared/no_ssh' = render 'shared/no_password' -- if prefer_readme? - = render 'projects/last_push' += render 'projects/last_push' = render "home_panel" -- cgit v1.2.1 From 6f0856535e242694805909b824e9e238372e3f65 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:16:04 +0200 Subject: Remove redundant helper method. --- app/helpers/preferences_helper.rb | 10 ++-------- app/views/projects/show.html.haml | 7 +++---- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/app/helpers/preferences_helper.rb b/app/helpers/preferences_helper.rb index 5a49ab8195c..c73cb3028ee 100644 --- a/app/helpers/preferences_helper.rb +++ b/app/helpers/preferences_helper.rb @@ -47,13 +47,7 @@ module PreferencesHelper Gitlab::ColorSchemes.for_user(current_user).css_class end - def prefer_readme? - !current_user || - current_user.project_view == 'readme' - end - - def current_user_default_project_view - (current_user && current_user.project_view) || - 'readme' + def default_project_view + current_user ? current_user.project_view : 'readme' end end diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index f8c06d8b06b..585caf674c9 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -27,7 +27,7 @@ = link_to project_files_path(@project) do = repository_size - - if !prefer_readme? && @repository.readme + - if default_project_view != 'readme' && @repository.readme %li = link_to 'Readme', readme_path(@project) @@ -67,9 +67,8 @@ .content-block.second-block.white = render 'projects/last_commit', commit: @repository.commit, project: @project -%section - %div{class: "project-show-#{current_user_default_project_view}"} - = render current_user_default_project_view +%div{class: "project-show-#{default_project_view}"} + = render default_project_view - if current_user - access = user_max_access_in_project(current_user, @project) -- cgit v1.2.1 From 6bd9a9fbf7aa33b74ad2522466eb84fbbc6aa7b5 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:16:40 +0200 Subject: Set vars used by tree view in project show action. --- app/controllers/projects_controller.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 7158d4b49ac..c81c7ea59c2 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -1,8 +1,11 @@ class ProjectsController < ApplicationController + include ExtractsPath + prepend_before_filter :render_go_import, only: [:show] skip_before_action :authenticate_user!, only: [:show, :activity] before_action :project, except: [:new, :create] before_action :repository, except: [:new, :create] + before_action :assign_ref_vars, :tree, only: [:show] # Authorize before_action :authorize_admin_project!, only: [:edit, :update, :destroy, :transfer, :archive, :unarchive] @@ -89,10 +92,7 @@ class ProjectsController < ApplicationController if current_user @membership = @project.project_member_by_id(current_user.id) end - @ref = "master" - @id = "master" - @commit = @project.repository.commit(@ref) - @tree = @project.repository.tree(@commit.id) + render :show end else @@ -228,4 +228,8 @@ class ProjectsController < ApplicationController render "go_import", layout: false end + + def get_id + project.repository.root_ref + end end -- cgit v1.2.1 From 7b26414c155796e60b4d6cff10a50d891f276ec2 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:17:13 +0200 Subject: Split up projects/tree/_tree partial. --- app/views/projects/tree/_tree.html.haml | 73 ------------------------- app/views/projects/tree/_tree_content.html.haml | 40 ++++++++++++++ app/views/projects/tree/_tree_header.html.haml | 32 +++++++++++ app/views/projects/tree/show.html.haml | 8 +-- 4 files changed, 76 insertions(+), 77 deletions(-) delete mode 100644 app/views/projects/tree/_tree.html.haml create mode 100644 app/views/projects/tree/_tree_content.html.haml create mode 100644 app/views/projects/tree/_tree_header.html.haml diff --git a/app/views/projects/tree/_tree.html.haml b/app/views/projects/tree/_tree.html.haml deleted file mode 100644 index 7ff48e32e60..00000000000 --- a/app/views/projects/tree/_tree.html.haml +++ /dev/null @@ -1,73 +0,0 @@ -.gray-content-block - %ul.breadcrumb.repo-breadcrumb - %li - = link_to namespace_project_tree_path(@project.namespace, @project, @ref) do - = @project.path - - tree_breadcrumbs(tree, 6) do |title, path| - %li - - if path - = link_to truncate(title, length: 40), namespace_project_tree_path(@project.namespace, @project, path) - - else - = link_to title, '#' - - if allowed_tree_edit? - %li - %span.dropdown - %a.dropdown-toggle.btn.add-to-tree{href: '#', "data-toggle" => "dropdown"} - = icon('plus') - %ul.dropdown-menu - %li - = link_to namespace_project_new_blob_path(@project.namespace, @project, @id), title: 'Create file', id: 'new-file-link' do - = icon('pencil fw') - Create file - %li - = link_to '#modal-upload-blob', { 'data-target' => '#modal-upload-blob', 'data-toggle' => 'modal'} do - = icon('file fw') - Upload file - %li.divider - %li - = link_to '#modal-create-new-dir', { 'data-target' => '#modal-create-new-dir', 'data-toggle' => 'modal'} do - = icon('folder fw') - New directory - -%div#tree-content-holder.tree-content-holder - .tree-table-holder - %table.table#tree-slider{class: "table_#{@hex_path} tree-table table-striped" } - %thead - %tr - %th Name - %th Last Update - %th.hidden-xs - .pull-left Last Commit - .last-commit.hidden-sm.pull-left -   - %i.fa.fa-angle-right -   - %small.light - = link_to @commit.short_id, namespace_project_commit_path(@project.namespace, @project, @commit) - – - = truncate(@commit.title, length: 50) - = link_to 'History', namespace_project_commits_path(@project.namespace, @project, @id), class: 'pull-right' - - - if @path.present? - %tr.tree-item - %td.tree-item-file-name - = link_to "..", namespace_project_tree_path(@project.namespace, @project, up_dir_path), class: 'prepend-left-10' - %td - %td.hidden-xs - - = render_tree(tree) - - - if tree.readme - = render "projects/tree/readme", readme: tree.readme - -%div.tree_progress - -- if allowed_tree_edit? - = render 'projects/blob/upload', title: 'Upload', placeholder: 'Upload new file', button_title: 'Upload file', form_path: namespace_project_create_blob_path(@project.namespace, @project, @id), method: :post - = render 'projects/blob/new_dir' - -:javascript - // Load last commit log for each file in tree - $('#tree-slider').waitForImages(function() { - ajaxGet("#{escape_javascript(@logs_path)}"); - }); diff --git a/app/views/projects/tree/_tree_content.html.haml b/app/views/projects/tree/_tree_content.html.haml new file mode 100644 index 00000000000..ed1f61e9077 --- /dev/null +++ b/app/views/projects/tree/_tree_content.html.haml @@ -0,0 +1,40 @@ +%div.tree-content-holder + .tree-table-holder + %table.table#tree-slider{class: "table_#{@hex_path} tree-table table-striped" } + %thead + %tr + %th Name + %th Last Update + %th.hidden-xs + .pull-left Last Commit + .last-commit.hidden-sm.pull-left +   + %i.fa.fa-angle-right +   + %small.light + = link_to @commit.short_id, namespace_project_commit_path(@project.namespace, @project, @commit) + – + = truncate(@commit.title, length: 50) + = link_to 'History', namespace_project_commits_path(@project.namespace, @project, @id), class: 'pull-right' + + - if @path.present? + %tr.tree-item + %td.tree-item-file-name + = link_to "..", namespace_project_tree_path(@project.namespace, @project, up_dir_path), class: 'prepend-left-10' + %td + %td.hidden-xs + + = render_tree(tree) + + - if tree.readme + = render "projects/tree/readme", readme: tree.readme + +- if allowed_tree_edit? + = render 'projects/blob/upload', title: 'Upload', placeholder: 'Upload new file', button_title: 'Upload file', form_path: namespace_project_create_blob_path(@project.namespace, @project, @id), method: :post + = render 'projects/blob/new_dir' + +:javascript + // Load last commit log for each file in tree + $('#tree-slider').waitForImages(function() { + ajaxGet("#{escape_javascript(@logs_path)}"); + }); diff --git a/app/views/projects/tree/_tree_header.html.haml b/app/views/projects/tree/_tree_header.html.haml new file mode 100644 index 00000000000..1115ca6b4ca --- /dev/null +++ b/app/views/projects/tree/_tree_header.html.haml @@ -0,0 +1,32 @@ +.tree-ref-holder + = render 'shared/ref_switcher', destination: 'tree', path: @path + +%ul.breadcrumb.repo-breadcrumb + %li + = link_to namespace_project_tree_path(@project.namespace, @project, @ref) do + = @project.path + - tree_breadcrumbs(tree, 6) do |title, path| + %li + - if path + = link_to truncate(title, length: 40), namespace_project_tree_path(@project.namespace, @project, path) + - else + = link_to title, '#' + - if allowed_tree_edit? + %li + %span.dropdown + %a.dropdown-toggle.btn.add-to-tree{href: '#', "data-toggle" => "dropdown"} + = icon('plus') + %ul.dropdown-menu + %li + = link_to namespace_project_new_blob_path(@project.namespace, @project, @id), title: 'Create file', id: 'new-file-link' do + = icon('pencil fw') + Create file + %li + = link_to '#modal-upload-blob', { 'data-target' => '#modal-upload-blob', 'data-toggle' => 'modal'} do + = icon('file fw') + Upload file + %li.divider + %li + = link_to '#modal-create-new-dir', { 'data-target' => '#modal-create-new-dir', 'data-toggle' => 'modal'} do + = icon('folder fw') + New directory diff --git a/app/views/projects/tree/show.html.haml b/app/views/projects/tree/show.html.haml index dec4677f830..ec14bd7f65a 100644 --- a/app/views/projects/tree/show.html.haml +++ b/app/views/projects/tree/show.html.haml @@ -6,12 +6,12 @@ = render 'projects/last_push' -.tree-ref-holder - = render 'shared/ref_switcher', destination: 'tree', path: @path - - if can? current_user, :download_code, @project .tree-download-holder = render 'projects/repositories/download_archive', ref: @ref, btn_class: 'btn-group pull-right hidden-xs hidden-sm', split_button: true #tree-holder.tree-holder.clearfix - = render "tree", tree: @tree + .gray-content-block.top-block + = render 'projects/tree/tree_header', tree: @tree + + = render 'projects/tree/tree_content', tree: @tree -- cgit v1.2.1 From a22fe254c18a64b1f145593781b2505d0fbdd46c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:17:31 +0200 Subject: Use same style for project readme, tree readme and regular blob. --- app/assets/stylesheets/pages/tree.scss | 8 -------- app/views/projects/_readme.html.haml | 15 +++++++-------- app/views/projects/tree/_readme.html.haml | 6 +++--- 3 files changed, 10 insertions(+), 19 deletions(-) diff --git a/app/assets/stylesheets/pages/tree.scss b/app/assets/stylesheets/pages/tree.scss index dadd86e88cc..ace371d7695 100644 --- a/app/assets/stylesheets/pages/tree.scss +++ b/app/assets/stylesheets/pages/tree.scss @@ -4,14 +4,6 @@ margin-right: -$gl-padding; } - .tree_progress { - display: none; - margin: 20px; - &.loading { - display: block; - } - } - .tree-table { margin-bottom: 0; diff --git a/app/views/projects/_readme.html.haml b/app/views/projects/_readme.html.haml index 5bc1999ec9d..0a1cecfdcdf 100644 --- a/app/views/projects/_readme.html.haml +++ b/app/views/projects/_readme.html.haml @@ -1,12 +1,11 @@ - if readme = @repository.readme - %article.readme-holder#README - .clearfix - .pull-right -   - - if can?(current_user, :push_code, @project) - = link_to namespace_project_edit_blob_path(@project.namespace, @project, tree_join(@repository.root_ref, readme.name)), class: 'light' do - %i.fa-align.fa.fa-pencil - .wiki + %article.file-holder.readme-holder + .file-title + = blob_icon readme.mode, readme.name + = link_to namespace_project_blob_path(@project.namespace, @project, tree_join(@repository.root_ref, readme.name)) do + %strong + = readme.name + .file-content.wiki = cache(readme_cache_key) do = render_readme(readme) - else diff --git a/app/views/projects/tree/_readme.html.haml b/app/views/projects/tree/_readme.html.haml index 7e9af19c8ba..3c5edf4b033 100644 --- a/app/views/projects/tree/_readme.html.haml +++ b/app/views/projects/tree/_readme.html.haml @@ -1,8 +1,8 @@ -%article.file-holder.readme-holder#README +%article.file-holder.readme-holder .file-title - = link_to '#README' do + = blob_icon readme.mode, readme.name + = link_to namespace_project_blob_path(@project.namespace, @project, tree_join(@repository.root_ref, readme.name)) do %strong - %i.fa.fa-file = readme.name .file-content.wiki = render_readme(readme) -- cgit v1.2.1 From f009b6e256cbb9493883949abd87cd41920ba054 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:18:28 +0200 Subject: Tweak styling of project home files list. --- app/assets/stylesheets/pages/projects.scss | 6 +----- app/views/projects/_files.html.haml | 13 ++++--------- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index db6864ed784..bc62532f8a0 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -543,10 +543,6 @@ pre.light-well { } } -.project-show-files { - padding-top: 20px; -} - .project-show-readme .readme-holder { - padding: 7px; + border-top: 0; } diff --git a/app/views/projects/_files.html.haml b/app/views/projects/_files.html.haml index 2a99708eb43..fa978325ddd 100644 --- a/app/views/projects/_files.html.haml +++ b/app/views/projects/_files.html.haml @@ -1,11 +1,6 @@ -= render 'projects/last_push' - -.tree-ref-holder - = render 'shared/ref_switcher', destination: 'tree', path: @path +#tree-holder.tree-holder.clearfix + .gray-content-block.second-block + = render 'projects/tree/tree_header', tree: @tree -- if can? current_user, :download_code, @project - .tree-download-holder - = render 'projects/repositories/download_archive', ref: @ref, btn_class: 'btn-group pull-right hidden-xs hidden-sm', split_button: true + = render 'projects/tree/tree_content', tree: @tree -#tree-holder.tree-holder.clearfix - = render "projects/tree/tree", tree: @tree -- cgit v1.2.1 From 33fd13c8f4909cbe52a8ec38701740f11f2fccbd Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:18:37 +0200 Subject: Remove border at bottom of readme. --- app/assets/stylesheets/framework/files.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 9dd77747884..8742d1c39b3 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -10,6 +10,10 @@ border-bottom: 1px solid #E7E9EE; margin-bottom: 1em; + &.readme-holder { + border-bottom: 0; + } + table { @extend .table; } -- cgit v1.2.1 From afb33acae6ed37e630b33b7ae18d516eb8c5fdfb Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:18:57 +0200 Subject: Put grey block around blob ref switcher and breadcrumb. --- app/views/projects/blob/_blob.html.haml | 31 +++++++++++++++++-------------- app/views/projects/blob/show.html.haml | 3 --- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml index a1ae1397584..42f632b38ef 100644 --- a/app/views/projects/blob/_blob.html.haml +++ b/app/views/projects/blob/_blob.html.haml @@ -1,19 +1,22 @@ -%ul.breadcrumb.repo-breadcrumb - %li - %i.fa.fa-angle-right - = link_to namespace_project_tree_path(@project.namespace, @project, @ref) do - = @project.path - - tree_breadcrumbs(@tree, 6) do |title, path| +.gray-content-block.top-block + .tree-ref-holder + = render 'shared/ref_switcher', destination: 'blob', path: @path + + %ul.breadcrumb.repo-breadcrumb %li - - if path - - if path.end_with?(@path) - = link_to namespace_project_blob_path(@project.namespace, @project, path) do - %strong - = truncate(title, length: 40) + = link_to namespace_project_tree_path(@project.namespace, @project, @ref) do + = @project.path + - tree_breadcrumbs(@tree, 6) do |title, path| + %li + - if path + - if path.end_with?(@path) + = link_to namespace_project_blob_path(@project.namespace, @project, path) do + %strong + = truncate(title, length: 40) + - else + = link_to truncate(title, length: 40), namespace_project_tree_path(@project.namespace, @project, path) - else - = link_to truncate(title, length: 40), namespace_project_tree_path(@project.namespace, @project, path) - - else - = link_to title, '#' + = link_to title, '#' %ul.blob-commit-info.hidden-xs - blob_commit = @repository.last_commit_for_path(@commit.id, blob.path) diff --git a/app/views/projects/blob/show.html.haml b/app/views/projects/blob/show.html.haml index fa4be4a1bc4..f52b89f6921 100644 --- a/app/views/projects/blob/show.html.haml +++ b/app/views/projects/blob/show.html.haml @@ -3,9 +3,6 @@ = render 'projects/last_push' -%div.tree-ref-holder - = render 'shared/ref_switcher', destination: 'blob', path: @path - %div#tree-holder.tree-holder = render 'blob', blob: @blob -- cgit v1.2.1 From 58cdfba9b9cc57998059f7ad78422d1d46b98fd7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:19:33 +0200 Subject: Tweak help text. --- app/views/profiles/preferences/show.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/profiles/preferences/show.html.haml b/app/views/profiles/preferences/show.html.haml index 01e285a8dfa..cc41d7dd813 100644 --- a/app/views/profiles/preferences/show.html.haml +++ b/app/views/profiles/preferences/show.html.haml @@ -38,7 +38,7 @@ .col-sm-10 = f.select :layout, layout_choices, {}, class: 'form-control' .help-block - Choose between fixed (max. 1200px) and fluid (100%) application layout + Choose between fixed (max. 1200px) and fluid (100%) application layout. .form-group = f.label :dashboard, class: 'control-label' do Default Dashboard @@ -52,6 +52,6 @@ .col-sm-10 = f.select :project_view, project_view_choices, {}, class: 'form-control' .help-block - Choose what content you want to see when visit project page + Choose what content you want to see on a project's home page. .panel-footer = f.submit 'Save', class: 'btn btn-save' -- cgit v1.2.1 From aebe0ddc33b93a66bd52b63a114c485791f15805 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 17 Oct 2015 19:27:02 +0200 Subject: Make spec names more clear --- spec/controllers/projects_controller_spec.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 590a3f5b441..b7ec5e48e85 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -25,23 +25,26 @@ describe ProjectsController do context "rendering default project view" do render_views - it "shold render the activity view", focus: true do + it "renders the activity view" do allow(controller).to receive(:current_user).and_return(user) allow(user).to receive(:project_view).and_return('activity') + get :show, namespace_id: public_project.namespace.path, id: public_project.path expect(response).to render_template('_activity') end - it "shold render the readme view", focus: true do + it "renders the readme view" do allow(controller).to receive(:current_user).and_return(user) allow(user).to receive(:project_view).and_return('readme') + get :show, namespace_id: public_project.namespace.path, id: public_project.path expect(response).to render_template('_readme') end - it "shold render the files view", focus: true do + it "renders the files view" do allow(controller).to receive(:current_user).and_return(user) allow(user).to receive(:project_view).and_return('files') + get :show, namespace_id: public_project.namespace.path, id: public_project.path expect(response).to render_template('_files') end -- cgit v1.2.1 From ff866faf2fca82c7ad7e2d70cba2cae56cc5cf7f Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 11:22:33 +0200 Subject: Only load tree when project has repository to prevent 404 --- app/controllers/projects_controller.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index c81c7ea59c2..bb2df275b77 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -5,7 +5,7 @@ class ProjectsController < ApplicationController skip_before_action :authenticate_user!, only: [:show, :activity] before_action :project, except: [:new, :create] before_action :repository, except: [:new, :create] - before_action :assign_ref_vars, :tree, only: [:show] + before_action :assign_ref_vars, :tree, only: [:show], if: :repo_exists? # Authorize before_action :authorize_admin_project!, only: [:edit, :update, :destroy, :transfer, :archive, :unarchive] @@ -229,6 +229,10 @@ class ProjectsController < ApplicationController render "go_import", layout: false end + def repo_exists? + project.repository_exists? && !project.empty_repo? + end + def get_id project.repository.root_ref end -- cgit v1.2.1 From f52b07cedcafc6cb5e92f549b7e7b4fab3b2ca83 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 11:22:40 +0200 Subject: Fix readme spec --- features/steps/project/project.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/features/steps/project/project.rb b/features/steps/project/project.rb index 15f77734cb2..d76891d5bde 100644 --- a/features/steps/project/project.rb +++ b/features/steps/project/project.rb @@ -86,13 +86,13 @@ class Spinach::Features::Project < Spinach::FeatureSteps end step 'I should see project "Forum" README' do - page.within('#README') do + page.within('.readme-holder') do expect(page).to have_content 'Sample repo for testing gitlab features' end end step 'I should see project "Shop" README' do - page.within('#README') do + page.within('.readme-holder') do expect(page).to have_content 'testme' end end -- cgit v1.2.1 From 6ad683bf13f820283a23cf44e15b241b0f4d7d87 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 11:58:14 +0200 Subject: Tweak email body. --- app/views/abuse_report_mailer/notify.text.haml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/abuse_report_mailer/notify.text.haml b/app/views/abuse_report_mailer/notify.text.haml index 70e4e6a3c6c..7dacf857035 100644 --- a/app/views/abuse_report_mailer/notify.text.haml +++ b/app/views/abuse_report_mailer/notify.text.haml @@ -1,5 +1,5 @@ -An abuse report was filed on `#{@abuse_report.user.username}` by `#{@abuse_report.reporter.username}`. +#{@abuse_report.user.name} (@#{@abuse_report.user.username}) was reported for abuse by #{@abuse_report.reporter.name} (@#{@abuse_report.reporter.username}). \ -= @abuse_report.message +> #{@abuse_report.message} \ -Abuse report admin screen: #{abuse_reports_url} \ No newline at end of file +View details: #{admin_abuse_reports_url} -- cgit v1.2.1 From dc170516edb4760d9dc8843830459fe8066dff42 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 11:58:24 +0200 Subject: Add HTML abuse report notification email. --- app/views/abuse_report_mailer/notify.html.haml | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 app/views/abuse_report_mailer/notify.html.haml diff --git a/app/views/abuse_report_mailer/notify.html.haml b/app/views/abuse_report_mailer/notify.html.haml new file mode 100644 index 00000000000..619533e09a7 --- /dev/null +++ b/app/views/abuse_report_mailer/notify.html.haml @@ -0,0 +1,11 @@ +%p + #{link_to @abuse_report.user.name, user_url(@abuse_report.user)} + (@#{@abuse_report.user.username}) was reported for abuse by + #{link_to @abuse_report.reporter.name, user_url(@abuse_report.reporter)} + (@#{@abuse_report.reporter.username}). + +%blockquote + = @abuse_report.message + +%p + = link_to "View details", abuse_reports_url -- cgit v1.2.1 From 9f6dc2a4b2e5eca01f5712bd7ec4d007ad4e57e5 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 11:58:45 +0200 Subject: Only pass abuse report ID to AbuseReportMailer. --- app/controllers/abuse_reports_controller.rb | 7 ++++--- app/mailers/abuse_report_mailer.rb | 10 +++++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/app/controllers/abuse_reports_controller.rb b/app/controllers/abuse_reports_controller.rb index 482ec5054ac..2f4054eaa11 100644 --- a/app/controllers/abuse_reports_controller.rb +++ b/app/controllers/abuse_reports_controller.rb @@ -9,11 +9,12 @@ class AbuseReportsController < ApplicationController @abuse_report.reporter = current_user if @abuse_report.save - message = "Thank you for your report. A GitLab administrator will look into it shortly." - redirect_to root_path, notice: message if current_application_settings.admin_notification_email.present? - AbuseReportMailer.delay.notify(@abuse_report, current_application_settings.admin_notification_email) + AbuseReportMailer.delay.notify(@abuse_report.id) end + + message = "Thank you for your report. A GitLab administrator will look into it shortly." + redirect_to root_path, notice: message else render :new end diff --git a/app/mailers/abuse_report_mailer.rb b/app/mailers/abuse_report_mailer.rb index c8b9c9c1628..f0c41f69a5c 100644 --- a/app/mailers/abuse_report_mailer.rb +++ b/app/mailers/abuse_report_mailer.rb @@ -1,8 +1,12 @@ class AbuseReportMailer < BaseMailer + include Gitlab::CurrentSettings - def notify(abuse_report, to_email) - @abuse_report = abuse_report + def notify(abuse_report_id) + @abuse_report = AbuseReport.find(abuse_report_id) - mail(to: to_email, subject: "[Gitlab] Abuse report filed for `#{@abuse_report.user.username}`") + mail( + to: current_application_settings.admin_notification_email, + subject: "#{@abuse_report.user.name} (#{@abuse_report.user.username}) was reported for abuse" + ) end end -- cgit v1.2.1 From 3b1c702572facf3aff58beb91b2c5de4903d7a83 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 11:58:53 +0200 Subject: Fix spec. --- spec/controllers/abuse_reports_controller_spec.rb | 86 ++++++++++++++--------- 1 file changed, 53 insertions(+), 33 deletions(-) diff --git a/spec/controllers/abuse_reports_controller_spec.rb b/spec/controllers/abuse_reports_controller_spec.rb index 6d157406a2b..10a2cc3c3a4 100644 --- a/spec/controllers/abuse_reports_controller_spec.rb +++ b/spec/controllers/abuse_reports_controller_spec.rb @@ -9,45 +9,65 @@ describe AbuseReportsController do sign_in(reporter) end - describe "with admin notification_email set" do - let(:admin_email) { "admin@example.com"} - before(:example) { allow(current_application_settings).to receive(:admin_notification_email).and_return(admin_email) } - - it "sends a notification email" do - post(:create, - abuse_report: { - user_id: user.id, - message: message - } - ) - - expect(response).to have_http_status(:redirect) - expect(flash[:notice]).to start_with("Thank you for your report") - - email = ActionMailer::Base.deliveries.last - - expect(email).to be_present - expect(email.subject).to eq("[Gitlab] Abuse report filed for `#{user.username}`") - expect(email.to).to eq([admin_email]) - expect(email.body).to include(message) - end - end + describe "POST create" do + context "with admin notification email set" do + let(:admin_email) { "admin@example.com"} - describe "without admin notification email set" do - before(:example) { allow(current_application_settings).to receive(:admin_notification_email).and_return(nil) } + before(:each) do + stub_application_setting(admin_notification_email: admin_email) + end - it "does not send a notification email" do - expect do - post(:create, + it "sends a notification email" do + post :create, abuse_report: { user_id: user.id, message: message } - ) - end.to_not change{ActionMailer::Base.deliveries} - expect(response).to have_http_status(:redirect) - expect(flash[:notice]).to start_with("Thank you for your report") + email = ActionMailer::Base.deliveries.last + + expect(email.to).to eq([admin_email]) + expect(email.subject).to include(user.username) + expect(email.text_part.body).to include(message) + end + + it "saves the abuse report" do + expect { + post :create, + abuse_report: { + user_id: user.id, + message: message + } + }.to change { AbuseReport.count }.by(1) + end + end + + context "without admin notification email set" do + before(:each) do + stub_application_setting(admin_notification_email: nil) + end + + it "does not send a notification email" do + expect { + post :create, + abuse_report: { + user_id: user.id, + message: message + } + + }.not_to change { ActionMailer::Base.deliveries.count } + end + + it "saves the abuse report" do + expect { + post :create, + abuse_report: { + user_id: user.id, + message: message + } + }.to change { AbuseReport.count }.by(1) + end end end -end \ No newline at end of file + +end -- cgit v1.2.1 From 551512b147f63a9fcab938603eb70c112e80fee7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 11:58:59 +0200 Subject: Validate admin notification email. --- app/models/application_setting.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index c8841178e93..05430c2ee18 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -44,6 +44,10 @@ class ApplicationSetting < ActiveRecord::Base allow_blank: true, format: { with: /\A#{URI.regexp(%w(http https))}\z/, message: "should be a valid url" } + validates :admin_notification_email, + allow_blank: true, + email: true + validates_each :restricted_visibility_levels do |record, attr, value| unless value.nil? value.each do |level| -- cgit v1.2.1 From 7ccfbcccef9a78f9dd469b22070663028ff0e972 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 11:59:07 +0200 Subject: Add help text to admin settings notification email. --- app/views/admin/application_settings/_form.html.haml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 791734dd80d..7a78526e09a 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -51,6 +51,8 @@ = f.label :admin_notification_email, class: 'control-label col-sm-2' .col-sm-10 = f.text_field :admin_notification_email, class: 'form-control' + .help-block + Abuse reports will be sent to this address if it is set. Abuse reports are always available in the admin area. %fieldset %legend Account and Limit Settings -- cgit v1.2.1 From 024b6fa11a3d0d4aed017997148524e0df1eb177 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 12:06:47 +0200 Subject: Add changelog item --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 8f17c5c2ba6..b18a08bf89f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -5,6 +5,7 @@ v 8.2.0 (unreleased) - Highlight comment based on anchor in URL v 8.1.0 (unreleased) + - Send an email to admin email when a user is reported for spam (Jonathan Rochkind) - Fix nonatomic database update potentially causing project star counts to go negative (Stan Hu) - Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu) - Speed up load times of issue detail pages by roughly 1.5x -- cgit v1.2.1 From 99b8568ff79b188d664de9744797ce4013e55526 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 12:19:30 +0200 Subject: Find correct group membership. --- app/controllers/projects_controller.rb | 3 +-- app/views/projects/buttons/_notifications.html.haml | 11 +++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index ffbd91324cb..1ea992c4e85 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -87,8 +87,7 @@ class ProjectsController < ApplicationController render 'projects/empty' else if current_user - @membership = @project.project_member_by_id(current_user.id) - @group_member = GroupMember.find_by(user_id: current_user.id) + @membership = @project.team.find_member(current_user.id) end render :show diff --git a/app/views/projects/buttons/_notifications.html.haml b/app/views/projects/buttons/_notifications.html.haml index 6a620e7c232..9783ff8431c 100644 --- a/app/views/projects/buttons/_notifications.html.haml +++ b/app/views/projects/buttons/_notifications.html.haml @@ -1,6 +1,5 @@ -- return unless [@membership, @group_member].any? - -- if @membership +- case @membership +- when ProjectMember = form_tag profile_notifications_path, method: :put, remote: true, class: 'inline', id: 'notification-form' do = hidden_field_tag :notification_type, 'project' = hidden_field_tag :notification_id, @membership.id @@ -14,8 +13,8 @@ - Notification.project_notification_levels.each do |level| = notification_list_item(level, @membership) -- elsif @group_member - .btn.btn-new.disabled#notifications-button +- when GroupMember + .btn.btn-new.disabled = icon('bell') - = notification_label(@group_member) + = notification_label(@membership) = icon('angle-down') -- cgit v1.2.1 From 1195ecd1985494704fcbd859078e5ba99182ece0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 12:19:36 +0200 Subject: Add tooltip. --- app/views/projects/buttons/_notifications.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/buttons/_notifications.html.haml b/app/views/projects/buttons/_notifications.html.haml index 9783ff8431c..0c298844912 100644 --- a/app/views/projects/buttons/_notifications.html.haml +++ b/app/views/projects/buttons/_notifications.html.haml @@ -14,7 +14,7 @@ = notification_list_item(level, @membership) - when GroupMember - .btn.btn-new.disabled + .btn.btn-new.disabled.has_tooltip{title: "To change the notification level, you need to be a member of the project itself, not only its group."} = icon('bell') = notification_label(@membership) = icon('angle-down') -- cgit v1.2.1 From eeea6ef25f222e3935ef4a86e59d982ba6758b9a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 12:19:42 +0200 Subject: Sentences end in periods. --- app/views/shared/_clone_panel.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/shared/_clone_panel.html.haml b/app/views/shared/_clone_panel.html.haml index b23b2f0d5eb..2e4aab36301 100644 --- a/app/views/shared/_clone_panel.html.haml +++ b/app/views/shared/_clone_panel.html.haml @@ -6,7 +6,7 @@ type: 'button', | class: "btn #{ 'active' if default_clone_protocol == 'ssh' }#{ ' has_tooltip' if current_user && current_user.require_ssh_key? }", | :"data-clone" => project.ssh_url_to_repo, | - :"data-title" => "Add an SSH key to your profile
to pull or push via SSH", + :"data-title" => "Add an SSH key to your profile
to pull or push via SSH.", :"data-html" => "true", :"data-container" => "body"} SSH @@ -15,7 +15,7 @@ type: 'button', | class: "btn #{ 'active' if default_clone_protocol == 'http' }#{ ' has_tooltip' if current_user && current_user.require_password? }", | :"data-clone" => project.http_url_to_repo, | - :"data-title" => "Set a password on your account
to pull or push via #{gitlab_config.protocol.upcase}", + :"data-title" => "Set a password on your account
to pull or push via #{gitlab_config.protocol.upcase}.", :"data-html" => "true", :"data-container" => "body"} = gitlab_config.protocol.upcase -- cgit v1.2.1 From ef9284636cbc63a7b6a8f8ddeabb1152ad4f6a96 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 12:21:28 +0200 Subject: Add changelog item --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 8f17c5c2ba6..2e86724b259 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -5,6 +5,7 @@ v 8.2.0 (unreleased) - Highlight comment based on anchor in URL v 8.1.0 (unreleased) + - Show notifications button when user is member of group rather than project (Grzegorz Bizon) - Fix nonatomic database update potentially causing project star counts to go negative (Stan Hu) - Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu) - Speed up load times of issue detail pages by roughly 1.5x -- cgit v1.2.1 From 42cbc7f813386dbf6d28868c9972ff38f01ad095 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 12:37:50 +0200 Subject: Tweak wording. --- app/controllers/projects_controller.rb | 4 +++- app/helpers/projects_helper.rb | 2 +- app/views/projects/edit.html.haml | 30 ++++++++++++++-------------- spec/controllers/projects_controller_spec.rb | 4 ++-- 4 files changed, 21 insertions(+), 19 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 77b3af9a5d0..12ef073e149 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -71,7 +71,7 @@ class ProjectsController < ApplicationController if @project.forked? @project.forked_project_link.destroy - flash[:notice] = 'Fork relationship has been removed.' + flash[:notice] = 'The fork relationship has been removed.' end end @@ -150,6 +150,7 @@ class ProjectsController < ApplicationController def archive return access_denied! unless can?(current_user, :archive_project, @project) + @project.archive! respond_to do |format| @@ -159,6 +160,7 @@ class ProjectsController < ApplicationController def unarchive return access_denied! unless can?(current_user, :archive_project, @project) + @project.unarchive! respond_to do |format| diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index e6e1bfd2c9b..472884e4ff2 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -71,7 +71,7 @@ module ProjectsHelper end def remove_fork_project_message(project) - "You are going to remove the fork relationship to the source project from #{@project.forked_from_project.namespace.try(:name)}. Are you ABSOLUTELY sure?" + "You are going to remove the fork relationship to source project #{@project.forked_from_project.name_with_namespace}. Are you ABSOLUTELY sure?" end def project_nav_tabs diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index ec58d0924b0..afbf88b5507 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -189,20 +189,20 @@ - else .nothing-here-block Only the project owner can transfer a project - - if @project.forked? && can?(current_user, :remove_fork_project, @project) - = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_namespace_project_path(@project.namespace, @project), method: :delete, remote: true, html: { class: 'transfer-project form-horizontal' }) do |f| - .panel.panel-default.panel.panel-danger - .panel-heading Remove fork relationship - .panel-body - %p - This will remove the relationship to the source project from - = link_to project_path(@project.forked_from_project) do - = @project.forked_from_project.namespace.try(:name) - %br - %strong Once removed it cannot be reversed through this interface. - = button_to 'Remove fork relationship', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) } - - elsif @project.forked? - .nothing-here-block Only the project owner can remove the fork relationship + - if @project.forked? + - if can?(current_user, :remove_fork_project, @project) + = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_namespace_project_path(@project.namespace, @project), method: :delete, remote: true, html: { class: 'transfer-project form-horizontal' }) do |f| + .panel.panel-default.panel.panel-danger + .panel-heading Remove fork relationship + .panel-body + %p + This will remove the fork relationship to source project + #{link_to @project.forked_from_project.name_with_namespace, project_path(@project.forked_from_project)}. + %br + %strong Once removed, the fork relationship cannot be restored and you will no longer be able to send merge requests to the source. + = button_to 'Remove fork relationship', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) } + - else + .nothing-here-block Only the project owner can remove the fork relationship. - if can?(current_user, :remove_project, @project) .panel.panel-default.panel.panel-danger @@ -216,7 +216,7 @@ = button_to 'Remove project', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_project_message(@project) } - else - .nothing-here-block Only project owner can remove a project + .nothing-here-block Only the project owner can remove a project. .save-project-loader.hide diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index e963e913512..9b0527a68d2 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -63,7 +63,7 @@ describe ProjectsController do end end - describe "PUT remove_fork" do + describe "DELETE remove_fork" do context 'when signed in' do before do sign_in(user) @@ -82,7 +82,7 @@ describe ProjectsController do id: project_fork.to_param, format: :js) expect(project_fork.forked?).to be_falsey - expect(flash[:notice]).to eq('Fork relationship has been removed.') + expect(flash[:notice]).to eq('The fork relationship has been removed.') expect(response).to render_template(:remove_fork) end end -- cgit v1.2.1 From 2e2a2a366fa5a7b1179bf34bf22128138e52e4c7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 13:08:08 +0200 Subject: Satisfy Rubocop --- spec/controllers/abuse_reports_controller_spec.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/spec/controllers/abuse_reports_controller_spec.rb b/spec/controllers/abuse_reports_controller_spec.rb index 10a2cc3c3a4..0faab8d7ff0 100644 --- a/spec/controllers/abuse_reports_controller_spec.rb +++ b/spec/controllers/abuse_reports_controller_spec.rb @@ -32,13 +32,13 @@ describe AbuseReportsController do end it "saves the abuse report" do - expect { + expect do post :create, abuse_report: { user_id: user.id, message: message } - }.to change { AbuseReport.count }.by(1) + end.to change { AbuseReport.count }.by(1) end end @@ -48,24 +48,23 @@ describe AbuseReportsController do end it "does not send a notification email" do - expect { + expect do post :create, abuse_report: { user_id: user.id, message: message } - - }.not_to change { ActionMailer::Base.deliveries.count } + end.not_to change { ActionMailer::Base.deliveries.count } end it "saves the abuse report" do - expect { + expect do post :create, abuse_report: { user_id: user.id, message: message } - }.to change { AbuseReport.count }.by(1) + end.to change { AbuseReport.count }.by(1) end end end -- cgit v1.2.1 From cc2b05adf80f55c9f0fe4b453f9f45e2b402c006 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 14:05:27 +0200 Subject: Fix bug where a push would only create cross references from the first commit. --- app/services/git_push_service.rb | 2 +- lib/gitlab/reference_extractor.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index e54044365b9..3de7bb9dcaa 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -79,7 +79,7 @@ class GitPushService authors = Hash.new do |hash, commit| email = commit.author_email - return hash[email] if hash.has_key?(email) + next hash[email] if hash.has_key?(email) hash[email] = commit_user(commit) end diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 333bd059055..da8df8a3025 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -27,7 +27,7 @@ module Gitlab def references @references ||= Hash.new do |references, type| type = type.to_sym - return references[type] if references.has_key?(type) + next references[type] if references.has_key?(type) references[type] = pipeline_result(type) end -- cgit v1.2.1 From f3a74556b15f7429749384a823b73253602454cf Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 14:12:50 +0200 Subject: Fix spec --- spec/features/projects_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index f3d51641ece..09fcff2444a 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -49,7 +49,7 @@ feature 'Project', feature: true do remove_with_confirm('Remove fork relationship', project.path) - expect(page).to have_content 'Fork relationship has been removed.' + expect(page).to have_content 'The fork relationship has been removed.' expect(project.forked?).to be_falsey expect(page).not_to have_content 'Remove fork relationship' end -- cgit v1.2.1 From aafb36616cbad9b3478964dffc10fca97c2f55bb Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 14:26:25 +0200 Subject: Fix schema [ci skip] --- db/schema.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 7d60e3cf9e3..886b05f3e56 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -416,7 +416,6 @@ ActiveRecord::Schema.define(version: 20151016195706) do end add_index "labels", ["project_id"], name: "index_labels_on_project_id", using: :btree - add_index "labels", ["title"], name: "index_labels_on_title", using: :btree create_table "members", force: true do |t| t.integer "access_level", null: false @@ -498,7 +497,6 @@ ActiveRecord::Schema.define(version: 20151016195706) do add_index "milestones", ["due_date"], name: "index_milestones_on_due_date", using: :btree add_index "milestones", ["project_id", "iid"], name: "index_milestones_on_project_id_and_iid", unique: true, using: :btree add_index "milestones", ["project_id"], name: "index_milestones_on_project_id", using: :btree - add_index "milestones", ["title"], name: "index_milestones_on_title", using: :btree create_table "namespaces", force: true do |t| t.string "name", null: false -- cgit v1.2.1 From d12e49c5916a42dafba32555405878f32c7ad254 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 15:07:02 +0200 Subject: Fix bug preventing mentioned issued from being closed when MR is merged using fast-forward merge. --- CHANGELOG | 1 + app/services/merge_requests/post_merge_service.rb | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index abf9ead6df7..699b44f00d7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -5,6 +5,7 @@ v 8.2.0 (unreleased) - Highlight comment based on anchor in URL v 8.1.0 (unreleased) + - Fix bug preventing mentioned issued from being closed when MR is merged using fast-forward merge. - Fix nonatomic database update potentially causing project star counts to go negative (Stan Hu) - Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu) - Speed up load times of issue detail pages by roughly 1.5x diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index aceb8cb9021..8f25c5e2496 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -6,6 +6,7 @@ module MergeRequests # class PostMergeService < MergeRequests::BaseService def execute(merge_request) + close_issues(merge_request) merge_request.mark_as_merged create_merge_event(merge_request, current_user) create_note(merge_request) @@ -15,6 +16,15 @@ module MergeRequests private + def close_issues(merge_request) + return unless merge_request.target_branch == project.default_branch + + closed_issues = merge_request.closes_issues(current_user) + closed_issues.each do |issue| + Issues::CloseService.new(project, current_user, {}).execute(issue, merge_request) + end + end + def create_merge_event(merge_request, current_user) EventCreateService.new.merge_mr(merge_request, current_user) end -- cgit v1.2.1 From 85264ab6a9fa11a2e325d72c02c00d4b8862f933 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 18 Oct 2015 15:24:04 +0200 Subject: Restore notification footer text. --- app/views/layouts/notify.html.haml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml index 2f7d7e86f56..854cda57c39 100644 --- a/app/views/layouts/notify.html.haml +++ b/app/views/layouts/notify.html.haml @@ -41,4 +41,8 @@ #{link_to "view it on GitLab", @target_url}. - else #{link_to "View it on GitLab", @target_url} + %br + You're receiving this email because of your account on #{link_to Gitlab.config.gitlab.host, root_url}. + If you'd like to receive fewer emails, you can adjust your notification settings. + = email_action @target_url -- cgit v1.2.1 From 405a10e9b8f94578b0417cb4fea0362aa139bac3 Mon Sep 17 00:00:00 2001 From: Jamie Mansfield Date: Sun, 18 Oct 2015 17:42:18 +0100 Subject: Fix spelling of Description --- app/views/projects/ci_services/index.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/ci_services/index.html.haml b/app/views/projects/ci_services/index.html.haml index c78b21884a3..c164b2d4bc0 100644 --- a/app/views/projects/ci_services/index.html.haml +++ b/app/views/projects/ci_services/index.html.haml @@ -6,7 +6,7 @@ %tr %th %th Service - %th Desription + %th Description %th Last edit - @services.sort_by(&:title).each do |service| %tr -- cgit v1.2.1 From 6ad78d3ab1fc0ea9f344810e22b4fa7e8d67b6f7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 19 Oct 2015 11:25:21 +0200 Subject: Move changelog item to 8.2 [ci skip] --- CHANGELOG | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 46fec4bd921..08a5030f725 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.2.0 (unreleased) - Show last project commit to default branch on project home page - Highlight comment based on anchor in URL + - Adds ability to remove the forked relationship from project settings screen. (Han Loong Liauw) v 8.1.0 (unreleased) - Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu) @@ -59,8 +60,6 @@ v 8.1.0 (unreleased) - Fix position of hamburger in header for smaller screens (Han Loong Liauw) - Fix bug where Emojis in Markdown would truncate remaining text (Sakata Sinji) - Persist filters when sorting on admin user page (Jerry Lukins) - - Adds ability to remove the forked relationship from project settings - screen. (Han Loong Liauw) - Allow dashboard and group issues/MRs to be filtered by label - Add spellcheck=false to certain input fields - Invalidate stored service password if the endpoint URL is changed -- cgit v1.2.1 From 4ff75e317935f990b90dcc5869afe8ebb2b6fee6 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 15 Oct 2015 18:10:35 +0200 Subject: Improve performance of sorting milestone issues This cuts down the time it takes to sort issues of a milestone by about 10x. In the previous setup the code would run a SQL query for every issue that had to be sorted. The new setup instead runs a single SQL query to update all the given issues at once. The attached benchmark used to run at around 60 iterations per second, using the new setup this hovers around 600 iterations per second. Timing wise a request to update a milestone with 40-something issues would take about 760 ms, in the new setup this only takes about 130 ms. Fixes #3066 --- CHANGELOG | 1 + app/controllers/projects/milestones_controller.rb | 6 +---- app/models/milestone.rb | 32 +++++++++++++++++++++++ spec/benchmarks/models/milestone_spec.rb | 17 ++++++++++++ spec/models/milestone_spec.rb | 28 ++++++++++++++++++++ 5 files changed, 79 insertions(+), 5 deletions(-) create mode 100644 spec/benchmarks/models/milestone_spec.rb diff --git a/CHANGELOG b/CHANGELOG index f8daa6d246c..7fefc90c541 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 8.2.0 (unreleased) - Show last project commit to default branch on project home page - Highlight comment based on anchor in URL - Adds ability to remove the forked relationship from project settings screen. (Han Loong Liauw) + - Improved performance of sorting milestone issues v 8.1.0 (unreleased) - Fix bug preventing mentioned issued from being closed when MR is merged using fast-forward merge. diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index 86f4a02a6e9..15506bd677a 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -75,11 +75,7 @@ class Projects::MilestonesController < Projects::ApplicationController end def sort_issues - @issues = @milestone.issues.where(id: params['sortable_issue']) - @issues.each do |issue| - issue.position = params['sortable_issue'].index(issue.id.to_s) + 1 - issue.save - end + @milestone.sort_issues(params['sortable_issue'].map(&:to_i)) render json: { saved: true } end diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 84acba30b6b..2ff16e2825c 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -105,4 +105,36 @@ class Milestone < ActiveRecord::Base def author_id nil end + + # Sorts the issues for the given IDs. + # + # This method runs a single SQL query using a CASE statement to update the + # position of all issues in the current milestone (scoped to the list of IDs). + # + # Given the ids [10, 20, 30] this method produces a SQL query something like + # the following: + # + # UPDATE issues + # SET position = CASE + # WHEN id = 10 THEN 1 + # WHEN id = 20 THEN 2 + # WHEN id = 30 THEN 3 + # ELSE position + # END + # WHERE id IN (10, 20, 30); + # + # This method expects that the IDs given in `ids` are already Fixnums. + def sort_issues(ids) + pairs = [] + + ids.each_with_index do |id, index| + pairs << id + pairs << index + 1 + end + + conditions = 'WHEN id = ? THEN ? ' * ids.length + + issues.where(id: ids). + update_all(["position = CASE #{conditions} ELSE position END", *pairs]) + end end diff --git a/spec/benchmarks/models/milestone_spec.rb b/spec/benchmarks/models/milestone_spec.rb new file mode 100644 index 00000000000..a94afc4c40d --- /dev/null +++ b/spec/benchmarks/models/milestone_spec.rb @@ -0,0 +1,17 @@ +require 'spec_helper' + +describe Milestone, benchmark: true do + describe '#sort_issues' do + let(:milestone) { create(:milestone) } + + let(:issue1) { create(:issue, milestone: milestone) } + let(:issue2) { create(:issue, milestone: milestone) } + let(:issue3) { create(:issue, milestone: milestone) } + + let(:issue_ids) { [issue3.id, issue2.id, issue1.id] } + + benchmark_subject { milestone.sort_issues(issue_ids) } + + it { is_expected.to iterate_per_second(500) } + end +end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index c88d5349663..77c58627322 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -140,4 +140,32 @@ describe Milestone do end end + describe '#sort_issues' do + let(:milestone) { create(:milestone) } + + let(:issue1) { create(:issue, milestone: milestone, position: 1) } + let(:issue2) { create(:issue, milestone: milestone, position: 2) } + let(:issue3) { create(:issue, milestone: milestone, position: 3) } + let(:issue4) { create(:issue, position: 42) } + + it 'sorts the given issues' do + milestone.sort_issues([issue3.id, issue2.id, issue1.id]) + + issue1.reload + issue2.reload + issue3.reload + + expect(issue1.position).to eq(3) + expect(issue2.position).to eq(2) + expect(issue3.position).to eq(1) + end + + it 'ignores issues not part of the milestone' do + milestone.sort_issues([issue3.id, issue2.id, issue1.id, issue4.id]) + + issue4.reload + + expect(issue4.position).to eq(42) + end + end end -- cgit v1.2.1 From 157d891615bbc9d1176b2149cfa5193b9aa4773c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 19 Oct 2015 11:40:13 +0200 Subject: Add changelog item --- CHANGELOG | 1 + app/controllers/projects_controller.rb | 2 ++ 2 files changed, 3 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index f8daa6d246c..cfb7a581e81 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 8.2.0 (unreleased) - Show last project commit to default branch on project home page - Highlight comment based on anchor in URL - Adds ability to remove the forked relationship from project settings screen. (Han Loong Liauw) + - Allow users to select the Files view as default project view (Cristian Bica) v 8.1.0 (unreleased) - Fix bug preventing mentioned issued from being closed when MR is merged using fast-forward merge. diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 73200396ecc..9f0cce468b1 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -246,6 +246,8 @@ class ProjectsController < ApplicationController project.repository_exists? && !project.empty_repo? end + # Override get_id from ExtractsPath, which returns the branch and file path + # for the blob/tree, which in this case is just the root of the default branch. def get_id project.repository.root_ref end -- cgit v1.2.1 From 8b8fbd4e7f51db32425dd30770c1efcea75c00f7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 19 Oct 2015 11:46:22 +0200 Subject: Rename confusing methods --- app/finders/issuable_finder.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index f00bb02d0fb..c407dfc163a 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -93,7 +93,7 @@ class IssuableFinder params[:milestone_title].present? end - def no_milestones? + def filter_by_no_milestone? milestones? && params[:milestone_title] == Milestone::None.title end @@ -114,7 +114,7 @@ class IssuableFinder params[:label_name].present? end - def no_labels? + def filter_by_no_label? labels? && params[:label_name] == Label::None.title end @@ -227,7 +227,7 @@ class IssuableFinder def by_milestone(items) if milestones? - if no_milestones? + if filter_by_no_milestone? items = items.where(milestone_id: [-1, nil]) else items = items.joins(:milestone).where(milestones: { title: params[:milestone_title] }) @@ -243,7 +243,7 @@ class IssuableFinder def by_label(items) if labels? - if no_labels? + if filter_by_no_label? items = items. joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{klass.name}' AND label_links.target_id = #{klass.table_name}.id"). where(label_links: { id: nil }) -- cgit v1.2.1 From 4ad64ab3f4705b7fa88f855d67e0d2d268c5e395 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 19 Oct 2015 10:32:15 -0700 Subject: Fix duplicate repositories in GitHub import page By default, all the current user's repositories are accessible via the /users endpoint. There's no need to traverse all the organization repositories as well. See: * http://www.rubydoc.info/github/pengwynn/octokit/Octokit/Client/Repositories#repositories-instance_method * https://developer.github.com/v3/repos/#list-your-repositories Closes #2523 --- CHANGELOG | 1 + app/controllers/import/github_controller.rb | 4 ---- spec/controllers/import/github_controller_spec.rb | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 634cf15f946..5671d8b1d81 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.2.0 (unreleased) + - Fix duplicate repositories in GitHub import page (Stan Hu) - Show last project commit to default branch on project home page - Highlight comment based on anchor in URL - Adds ability to remove the forked relationship from project settings screen. (Han Loong Liauw) diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index aae77d384c6..67bf4190e7e 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -11,10 +11,6 @@ class Import::GithubController < Import::BaseController def status @repos = client.repos - client.orgs.each do |org| - @repos += client.org_repos(org.login) - end - @already_added_projects = current_user.created_projects.where(import_type: "github") already_added_projects_names = @already_added_projects.pluck(:import_source) diff --git a/spec/controllers/import/github_controller_spec.rb b/spec/controllers/import/github_controller_spec.rb index 766be578f7f..bbf8adef534 100644 --- a/spec/controllers/import/github_controller_spec.rb +++ b/spec/controllers/import/github_controller_spec.rb @@ -41,7 +41,7 @@ describe Import::GithubController do it "assigns variables" do @project = create(:project, import_type: 'github', creator_id: user.id) - stub_client(repos: [@repo], orgs: [@org], org_repos: [@org_repo]) + stub_client(repos: [@repo, @org_repo], orgs: [@org], org_repos: [@org_repo]) get :status -- cgit v1.2.1 From d02a467d64679dd8ff02b5f4554ae5700f0bdfd7 Mon Sep 17 00:00:00 2001 From: Benny Schimmer Date: Mon, 19 Oct 2015 22:14:22 +0200 Subject: Fix regex in redis version check --- lib/tasks/gitlab/check.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tasks/gitlab/check.rake b/lib/tasks/gitlab/check.rake index 606bf241db7..2e73f792a9d 100644 --- a/lib/tasks/gitlab/check.rake +++ b/lib/tasks/gitlab/check.rake @@ -335,7 +335,7 @@ namespace :gitlab do print "Redis version >= #{min_redis_version}? ... " redis_version = run(%W(redis-cli --version)) - redis_version = redis_version.try(:match, /redis-cli (.*)/) + redis_version = redis_version.try(:match, /redis-cli (\d+\.\d+\.\d+)/) if redis_version && (Gem::Version.new(redis_version[1]) > Gem::Version.new(min_redis_version)) puts "yes".green -- cgit v1.2.1 From ffacb01be202f3b30d79116f84d0a2c7f37c96e2 Mon Sep 17 00:00:00 2001 From: Maurice Mohlek Date: Tue, 20 Oct 2015 09:40:21 +0200 Subject: Update CHANGELOG --- CHANGELOG | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 634cf15f946..9f4dcf5fc53 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -7,6 +7,10 @@ v 8.2.0 (unreleased) - Improved performance of sorting milestone issues - Allow users to select the Files view as default project view (Cristian Bica) +v 8.0.5 + - Correct lookup-by-email for LDAP logins + - Fix loading spinner sometimes not being hidden on Merge Request tab switches + v 8.1.0 (unreleased) - Send an email to admin email when a user is reported for spam (Jonathan Rochkind) - Show notifications button when user is member of group rather than project (Grzegorz Bizon) -- cgit v1.2.1 From b0a3ece265350b8d02218746ff636a8363987580 Mon Sep 17 00:00:00 2001 From: Maurice Mohlek Date: Tue, 20 Oct 2015 09:44:13 +0200 Subject: Fix order --- CHANGELOG | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 9f4dcf5fc53..d6b5e275fec 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -7,10 +7,6 @@ v 8.2.0 (unreleased) - Improved performance of sorting milestone issues - Allow users to select the Files view as default project view (Cristian Bica) -v 8.0.5 - - Correct lookup-by-email for LDAP logins - - Fix loading spinner sometimes not being hidden on Merge Request tab switches - v 8.1.0 (unreleased) - Send an email to admin email when a user is reported for spam (Jonathan Rochkind) - Show notifications button when user is member of group rather than project (Grzegorz Bizon) @@ -83,6 +79,10 @@ v 8.1.0 (unreleased) - Optimize query when filtering on issuables (Zeger-Jan van de Weg) - Fix padding of outdated discussion item. +v 8.0.5 + - Correct lookup-by-email for LDAP logins + - Fix loading spinner sometimes not being hidden on Merge Request tab switches + v 8.0.4 - Fix Message-ID header to be RFC 2111-compliant to prevent e-mails being dropped (Stan Hu) - Fix referrals for :back and relative URL installs -- cgit v1.2.1