From b1ef1aa59f5ccb78be6d2462b56ed6bafebe65c0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 11:37:45 +0100 Subject: [PATCH 01/15] Slightly refactor ReferenceExtractor. --- app/models/concerns/mentionable.rb | 8 ++- lib/gitlab/closing_issue_extractor.rb | 6 +-- lib/gitlab/reference_extractor.rb | 70 +++++++++++++-------------- 3 files changed, 41 insertions(+), 43 deletions(-) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index d96e07034ec..52eb87d1dbc 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -65,12 +65,10 @@ module Mentionable # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference. def references(p = project, text = mentionable_text) return [] if text.blank? - ext = Gitlab::ReferenceExtractor.new - ext.analyze(text, p) + ext = Gitlab::ReferenceExtractor.new(p) + ext.analyze(text) - (ext.issues_for(p) + - ext.merge_requests_for(p) + - ext.commits_for(p)).uniq - [local_reference] + (ext.issues + ext.merge_requests + ext.commits).uniq - [local_reference] end # Create a cross-reference Note for each GFM reference to another Mentionable found in +mentionable_text+. diff --git a/lib/gitlab/closing_issue_extractor.rb b/lib/gitlab/closing_issue_extractor.rb index a9fd59f03d9..5643d60c807 100644 --- a/lib/gitlab/closing_issue_extractor.rb +++ b/lib/gitlab/closing_issue_extractor.rb @@ -9,9 +9,9 @@ module Gitlab md = message.scan(ISSUE_CLOSING_REGEX) md.each do |ref| - extractor = Gitlab::ReferenceExtractor.new - extractor.analyze(ref[0], project) - issues += extractor.issues_for(project) + extractor = Gitlab::ReferenceExtractor.new(project) + extractor.analyze(ref[0]) + issues += extractor.issues end end diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 1058d4c43d9..90e04818719 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -1,89 +1,89 @@ module Gitlab # Extract possible GFM references from an arbitrary String for further processing. class ReferenceExtractor - attr_accessor :users, :labels, :issues, :merge_requests, :snippets, :commits, :commit_ranges + attr_accessor :project, :references include Markdown - def initialize - @users, @labels, @issues, @merge_requests, @snippets, @commits, @commit_ranges = - [], [], [], [], [], [], [] + def initialize(project) + @project = project + + @references = Hash.new { [] } end - def analyze(string, project) - text = string.dup + def analyze(text) + text = text.dup # Remove preformatted/code blocks so that references are not included text.gsub!(%r{
.*?
|.*?}m) { |match| '' } text.gsub!(%r{^```.*?^```}m) { |match| '' } - parse_references(text, project) + parse_references(text) end # Given a valid project, resolve the extracted identifiers of the requested type to # model objects. - def users_for(project) - users.map do |entry| + def users + references[:users].map do |entry| project.users.where(username: entry[:id]).first - end.reject(&:nil?) + end.compact end - def labels_for(project = nil) - labels.map do |entry| + def labels + references[:labels].map do |entry| project.labels.where(id: entry[:id]).first - end.reject(&:nil?) + end.compact end - def issues_for(project = nil) - issues.map do |entry| - if should_lookup?(project, entry[:project]) + def issues + references[:issues].map do |entry| + if should_lookup?(entry[:project]) entry[:project].issues.where(iid: entry[:id]).first end - end.reject(&:nil?) + end.compact end - def merge_requests_for(project = nil) - merge_requests.map do |entry| - if should_lookup?(project, entry[:project]) + def merge_requests + references[:merge_requests].map do |entry| + if should_lookup?(entry[:project]) entry[:project].merge_requests.where(iid: entry[:id]).first end - end.reject(&:nil?) + end.compact end - def snippets_for(project) - snippets.map do |entry| + def snippets + references[:snippets].map do |entry| project.snippets.where(id: entry[:id]).first - end.reject(&:nil?) + end.compact end - def commits_for(project = nil) - commits.map do |entry| + def commits + references[:commits].map do |entry| repo = entry[:project].repository if entry[:project] - if should_lookup?(project, entry[:project]) + if should_lookup?(entry[:project]) repo.commit(entry[:id]) if repo end - end.reject(&:nil?) + end.compact end - def commit_ranges_for(project = nil) - commit_ranges.map do |entry| + def commit_ranges + references[:commit_ranges].map do |entry| repo = entry[:project].repository if entry[:project] - if repo && should_lookup?(project, entry[:project]) + if repo && should_lookup?(entry[:project]) from_id, to_id = entry[:id].split(/\.{2,3}/, 2) [repo.commit(from_id), repo.commit(to_id)] end - end.reject(&:nil?) + end.compact end private def reference_link(type, identifier, project, _) - # Append identifier to the appropriate collection. - send("#{type}s") << { project: project, id: identifier } + references[type] << { project: project, id: identifier } end - def should_lookup?(project, entry_project) + def should_lookup?(entry_project) if entry_project.nil? false else From ca58e369c9f2a72402cfcf4d86d29c115b1b909c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 11:38:22 +0100 Subject: [PATCH 02/15] Only allow user to reference objects they have access to. --- app/helpers/gitlab_markdown_helper.rb | 2 +- lib/gitlab/markdown.rb | 38 ++++++++++++++------------- lib/gitlab/reference_extractor.rb | 14 +++++++--- 3 files changed, 32 insertions(+), 22 deletions(-) diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 7ca3f058636..a4157d62533 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -19,7 +19,7 @@ module GitlabMarkdownHelper escape_once(body) end - gfm_body = gfm(escaped_body, @project, html_options) + gfm_body = gfm(escaped_body, @project, current_user, html_options) gfm_body.gsub!(%r{.*?}m) do |match| "#{match}#{link_to("", url, html_options)[0..-5]}" # "".length +1 diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 1dbea48ac14..7a3c8823e4d 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -39,7 +39,7 @@ module Gitlab # text - the source text # project - the project # html_options - extra options for the reference links as given to link_to - def gfm(text, project = @project, html_options = {}) + def gfm(text, project = @project, user = current_user, html_options = {}) gfm_with_options(text, {}, project, html_options) end @@ -51,7 +51,7 @@ module Gitlab # - reference_only_path - Use relative path for reference links # project - the project # html_options - extra options for the reference links as given to link_to - def gfm_with_options(text, options = {}, project = @project, html_options = {}) + def gfm_with_options(text, options = {}, project = @project, user = current_user, html_options = {}) return text if text.nil? # Duplicate the string so we don't alter the original, then call to_str @@ -78,7 +78,7 @@ module Gitlab # TODO: add popups with additional information - text = parse(text, project) + text = parse(text, project, user) # Insert pre block extractions text.gsub!(/\{gfm-extraction-(\h{32})\}/) do @@ -155,8 +155,8 @@ module Gitlab # text - Text to parse # # Returns parsed text - def parse(text, project = @project) - parse_references(text, project) if project + def parse(text, project = @project, user = current_user) + parse_references(text, project, user) if project text end @@ -182,7 +182,7 @@ module Gitlab TYPES = [:user, :issue, :label, :merge_request, :snippet, :commit, :commit_range].freeze - def parse_references(text, project = @project) + def parse_references(text, project = @project, user = current_user) # parse reference links text.gsub!(REFERENCE_PATTERN) do |match| type = TYPES.select{|t| !$~[t].nil?}.first @@ -192,11 +192,12 @@ module Gitlab project_path = $LAST_MATCH_INFO[:project] if project_path actual_project = ::Project.find_with_namespace(project_path) + actual_project ||= nil unless can?(user, :read_project, actual_project) project_prefix = project_path end parse_result($LAST_MATCH_INFO, type, - actual_project, project_prefix) || match + actual_project, user, project_prefix) || match end end @@ -204,7 +205,7 @@ module Gitlab # link. Returns nil if +type+ is nil, if the match string is an HTML # entity, if the reference is invalid, or if the matched text includes an # invalid project path. - def parse_result(match_info, type, project, project_prefix) + def parse_result(match_info, type, project, user, project_prefix) prefix = match_info[:prefix] suffix = match_info[:suffix] @@ -212,7 +213,7 @@ module Gitlab return nil if project.nil? && !project_prefix.nil? identifier = match_info[type] - ref_link = reference_link(type, identifier, project, project_prefix) + ref_link = reference_link(type, identifier, project, user, project_prefix) if ref_link "#{prefix}#{ref_link}#{suffix}" @@ -233,11 +234,11 @@ module Gitlab # identifier - Object identifier (Issue ID, SHA hash, etc.) # # Returns string rendered by the processing method - def reference_link(type, identifier, project = @project, prefix_text = nil) + def reference_link(type, identifier, project = @project, user = current_user, prefix_text = nil) send("reference_#{type}", identifier, project, prefix_text) end - def reference_user(identifier, project = @project, _ = nil) + def reference_user(identifier, project = @project, user = current_user, _ = nil) link_options = html_options.merge( class: "gfm gfm-project_member #{html_options[:class]}" ) @@ -251,6 +252,7 @@ module Gitlab elsif namespace = Namespace.find_by(path: identifier) url = if namespace.is_a?(Group) + return nil unless can?(user, :read_group, namespace) group_url(identifier, only_path: options[:reference_only_path]) else user_url(identifier, only_path: options[:reference_only_path]) @@ -260,7 +262,7 @@ module Gitlab end end - def reference_label(identifier, project = @project, _ = nil) + def reference_label(identifier, project = @project, user = current_user, _ = nil) if label = project.labels.find_by(id: identifier) link_options = html_options.merge( class: "gfm gfm-label #{html_options[:class]}" @@ -273,7 +275,7 @@ module Gitlab end end - def reference_issue(identifier, project = @project, prefix_text = nil) + def reference_issue(identifier, project = @project, user = current_user, prefix_text = nil) if project.default_issues_tracker? if project.issue_exists? identifier url = url_for_issue(identifier, project, only_path: options[:reference_only_path]) @@ -293,7 +295,7 @@ module Gitlab end end - def reference_merge_request(identifier, project = @project, prefix_text = nil) + def reference_merge_request(identifier, project = @project, user = current_user, prefix_text = nil) if merge_request = project.merge_requests.find_by(iid: identifier) link_options = html_options.merge( title: "Merge Request: #{merge_request.title}", @@ -306,7 +308,7 @@ module Gitlab end end - def reference_snippet(identifier, project = @project, _ = nil) + def reference_snippet(identifier, project = @project, user = current_user, _ = nil) if snippet = project.snippets.find_by(id: identifier) link_options = html_options.merge( title: "Snippet: #{snippet.title}", @@ -321,7 +323,7 @@ module Gitlab end end - def reference_commit(identifier, project = @project, prefix_text = nil) + def reference_commit(identifier, project = @project, user = current_user, prefix_text = nil) if project.valid_repo? && commit = project.repository.commit(identifier) link_options = html_options.merge( title: commit.link_title, @@ -337,7 +339,7 @@ module Gitlab end end - def reference_commit_range(identifier, project = @project, prefix_text = nil) + def reference_commit_range(identifier, project = @project, user = current_user, prefix_text = nil) from_id, to_id = identifier.split(/\.{2,3}/, 2) inclusive = identifier !~ /\.{3}/ @@ -363,7 +365,7 @@ module Gitlab end end - def reference_external_issue(identifier, project = @project, prefix_text = nil) + def reference_external_issue(identifier, project = @project, user = current_user, prefix_text = nil) url = url_for_issue(identifier, project, only_path: options[:reference_only_path]) title = project.external_issue_tracker.title diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 90e04818719..3bf6f14a7bc 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -1,16 +1,24 @@ module Gitlab # Extract possible GFM references from an arbitrary String for further processing. class ReferenceExtractor - attr_accessor :project, :references + attr_accessor :project, :current_user, :references include Markdown - def initialize(project) + def initialize(project, current_user = nil) @project = project + @current_user = user @references = Hash.new { [] } end + def can?(user, action, subject) + # When extracting references, no user means access to everything. + return true if user.nil? + + Ability.abilities.allowed?(user, action, subject) + end + def analyze(text) text = text.dup @@ -79,7 +87,7 @@ module Gitlab private - def reference_link(type, identifier, project, _) + def reference_link(type, identifier, project, user, _) references[type] << { project: project, id: identifier } end From d2bd60675913bba47a6c3fb43a282da6bfe20646 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 11:45:06 +0100 Subject: [PATCH 03/15] Refactor Mentionable mentioned users to use ReferenceExtractor. --- app/models/concerns/mentionable.rb | 23 ++++++----------------- lib/gitlab/reference_extractor.rb | 12 ++++++++++-- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 52eb87d1dbc..db75a34f592 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -43,28 +43,17 @@ module Mentionable end def mentioned_users - users = [] - return users if mentionable_text.blank? - has_project = self.respond_to? :project - matches = mentionable_text.scan(/@[a-zA-Z][a-zA-Z0-9_\-\.]*/) - matches.each do |match| - identifier = match.delete "@" - if identifier == "all" - users.push(*project.team.members.flatten) - elsif namespace = Namespace.find_by(path: identifier) - if namespace.is_a?(Group) - users.push(*namespace.users) - else - users << namespace.owner - end - end - end - users.uniq + return [] if mentionable_text.blank? + + ext = Gitlab::ReferenceExtractor.new(self.project) + ext.analyze(text) + ext.users.uniq end # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference. def references(p = project, text = mentionable_text) return [] if text.blank? + ext = Gitlab::ReferenceExtractor.new(p) ext.analyze(text) diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 3bf6f14a7bc..422d35aceb7 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -34,8 +34,16 @@ module Gitlab def users references[:users].map do |entry| - project.users.where(username: entry[:id]).first - end.compact + if entry[:id] == "all" + project.team.members.flatten + elsif namespace = Namespace.find_by(path: entry[:id]) + if namespace.is_a?(Group) + namespace.users + else + namespace.owner + end + end + end.flatten.compact end def labels From 403b727138e22ce8ba83332ab03fa21fb7f72a1c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 11:53:23 +0100 Subject: [PATCH 04/15] Slightly refactor getting note notification recipients. --- app/models/concerns/mentionable.rb | 2 +- app/services/notification_service.rb | 37 +++++++++++++++++----------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index db75a34f592..1ab0a28fddd 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -53,7 +53,7 @@ module Mentionable # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference. def references(p = project, text = mentionable_text) return [] if text.blank? - + ext = Gitlab::ReferenceExtractor.new(p) ext.analyze(text) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index cc5853144c5..42547f6f481 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -123,32 +123,29 @@ class NotificationService return true if note.note.start_with?('Status changed to closed') return true if note.cross_reference? && note.system == true - opts = { noteable_type: note.noteable_type, project_id: note.project_id } - target = note.noteable - if target.respond_to?(:participants) - recipients = target.participants - else - recipients = note.mentioned_users - end + recipients = [] if note.commit_id.present? - opts.merge!(commit_id: note.commit_id) recipients << note.commit_author - else - opts.merge!(noteable_id: note.noteable_id) end # Get users who left comment in thread - recipients = recipients.concat(User.where(id: Note.where(opts).pluck(:author_id))) + recipients = recipients.concat(noteable_commenters(note)) # Merge project watchers recipients = recipients.concat(project_watchers(note.project)).compact.uniq - # Reject mention users unless mentioned in comment - recipients = reject_mention_users(recipients - note.mentioned_users, note.project) - recipients = recipients + note.mentioned_users + # Reject users with Mention notification level + recipients = reject_mention_users(recipients, note.project) + + # Add explicitly mentioned users + if target.respond_to?(:participants) + recipients = recipients.concat(target.participants) + else + recipients = recipients.concat(note.mentioned_users) + end # Reject mutes users recipients = reject_muted_users(recipients, note.project) @@ -195,6 +192,18 @@ class NotificationService protected + def noteable_commenters(note) + opts = { noteable_type: note.noteable_type, project_id: note.project_id } + + if note.commit_id.present? + opts.merge!(commit_id: note.commit_id) + else + opts.merge!(noteable_id: note.noteable_id) + end + + User.where(id: Note.where(opts).pluck(:author_id)) + end + # Get project users with WATCH notification level def project_watchers(project) project_members = project_member_notification(project) From d9698628d9042e820917e1144535888da147c228 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 12:16:34 +0100 Subject: [PATCH 05/15] Add Commit#author and #committer. --- app/helpers/commits_helper.rb | 7 ++++--- app/models/commit.rb | 8 ++++++++ app/services/git_push_service.rb | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 5aae697e2f0..d13d80be293 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -134,12 +134,13 @@ module CommitsHelper # avatar: true will prepend the avatar image # size: size of the avatar image in px def commit_person_link(commit, options = {}) + user = commit.send(options[:source]) + source_name = clean(commit.send "#{options[:source]}_name".to_sym) source_email = clean(commit.send "#{options[:source]}_email".to_sym) - user = User.find_for_commit(source_email, source_name) - person_name = user.nil? ? source_name : user.name - person_email = user.nil? ? source_email : user.email + person_name = user.try(:name) || source_name + person_email = user.try(:email) || source_email text = if options[:avatar] diff --git a/app/models/commit.rb b/app/models/commit.rb index e0461809e10..481300171be 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -126,6 +126,14 @@ class Commit "commit #{id}" end + def author + User.find_for_commit(author_email, author_name) + end + + def committer + User.find_for_commit(committer_email, committer_name) + end + def method_missing(m, *args, &block) @raw.send(m, *args, &block) end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 1f0b29dff5e..bb066e39f6b 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -127,6 +127,6 @@ class GitPushService end def commit_user(commit) - User.find_for_commit(commit.author_email, commit.author_name) || user + commit.author || user end end From 756e7aa8c347ccf211db03a7fa16ab33c021f820 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 12:18:15 +0100 Subject: [PATCH 06/15] Don't allow full access to guests in ReferenceExtractor --- lib/gitlab/reference_extractor.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 422d35aceb7..719274394f0 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -13,9 +13,6 @@ module Gitlab end def can?(user, action, subject) - # When extracting references, no user means access to everything. - return true if user.nil? - Ability.abilities.allowed?(user, action, subject) end From e62b5a2b072eb1bc8587b095e906bd194475eacc Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 12:19:34 +0100 Subject: [PATCH 07/15] Only allow user to see participants from groups they have access to. --- app/models/concerns/issuable.rb | 8 ++++---- app/models/concerns/mentionable.rb | 4 ++-- app/services/projects/participants_service.rb | 4 ++-- app/views/projects/issues/_discussion.html.haml | 4 ++-- .../projects/merge_requests/show/_participants.html.haml | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 88ac83744df..478134dff68 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -118,16 +118,16 @@ module Issuable end # Return all users participating on the discussion - def participants + def participants(current_user = self.author) users = [] users << author users << assignee if is_assigned? mentions = [] - mentions << self.mentioned_users + mentions << self.mentioned_users(current_user) notes.each do |note| users << note.author - mentions << note.mentioned_users + mentions << note.mentioned_users(current_user) end users.concat(mentions.reduce([], :|)).uniq @@ -140,7 +140,7 @@ module Issuable return subscription.subscribed end - participants.include?(user) + participants(user).include?(user) end def toggle_subscription(user) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 1ab0a28fddd..41bcfa6be21 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -42,10 +42,10 @@ module Mentionable Note.cross_reference_exists?(target, local_reference) end - def mentioned_users + def mentioned_users(current_user = nil) return [] if mentionable_text.blank? - ext = Gitlab::ReferenceExtractor.new(self.project) + ext = Gitlab::ReferenceExtractor.new(self.project, current_user) ext.analyze(text) ext.users.uniq end diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb index bcbacbff562..bcdde0950c5 100644 --- a/app/services/projects/participants_service.rb +++ b/app/services/projects/participants_service.rb @@ -21,10 +21,10 @@ module Projects users = case type when "Issue" issue = @project.issues.find_by_iid(id) - issue ? issue.participants : [] + issue ? issue.participants(user) : [] when "MergeRequest" merge_request = @project.merge_requests.find_by_iid(id) - merge_request ? merge_request.participants : [] + merge_request ? merge_request.participants(user) : [] when "Commit" author_ids = Note.for_commit_id(id).pluck(:author_id).uniq User.where(id: author_ids) diff --git a/app/views/projects/issues/_discussion.html.haml b/app/views/projects/issues/_discussion.html.haml index 0d3028d50b4..288b48f4583 100644 --- a/app/views/projects/issues/_discussion.html.haml +++ b/app/views/projects/issues/_discussion.html.haml @@ -9,8 +9,8 @@ .votes-holder.pull-right #votes= render 'votes/votes_block', votable: @issue .participants - %span= pluralize(@issue.participants.count, 'participant') - - @issue.participants.each do |participant| + %span= pluralize(@issue.participants(current_user).count, 'participant') + - @issue.participants(current_user).each do |participant| = link_to_member(@project, participant, name: false, size: 24) .voting_notes#notes= render "projects/notes/notes_with_form" %aside.col-md-3 diff --git a/app/views/projects/merge_requests/show/_participants.html.haml b/app/views/projects/merge_requests/show/_participants.html.haml index 4f34af1737d..9c93fa55fe6 100644 --- a/app/views/projects/merge_requests/show/_participants.html.haml +++ b/app/views/projects/merge_requests/show/_participants.html.haml @@ -1,4 +1,4 @@ .participants - %span #{@merge_request.participants.count} participants - - @merge_request.participants.each do |participant| + %span #{@merge_request.participants(current_user).count} participants + - @merge_request.participants(current_user).each do |participant| = link_to_member(@project, participant, name: false, size: 24) From 65bb0c34066eef77647e1d60e6bac7a12993cd93 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 12:19:48 +0100 Subject: [PATCH 08/15] Only allow users to cross-reference and close issues they have access to. --- app/models/commit.rb | 4 ++-- app/models/concerns/mentionable.rb | 6 +++--- app/models/merge_request.rb | 6 +++--- app/services/git_push_service.rb | 4 ++-- lib/gitlab/closing_issue_extractor.rb | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index 481300171be..084cf420079 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -117,8 +117,8 @@ class Commit # Discover issues should be closed when this commit is pushed to a project's # default branch. - def closes_issues(project) - Gitlab::ClosingIssueExtractor.closed_by_message_in_project(safe_message, project) + def closes_issues(project, current_user = self.committer) + Gitlab::ClosingIssueExtractor.closed_by_message_in_project(safe_message, project, current_user) end # Mentionable override. diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 41bcfa6be21..bf0465728e7 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -51,10 +51,10 @@ module Mentionable end # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference. - def references(p = project, text = mentionable_text) + def references(p = project, current_user = self.author, text = mentionable_text) return [] if text.blank? - ext = Gitlab::ReferenceExtractor.new(p) + ext = Gitlab::ReferenceExtractor.new(p, current_user) ext.analyze(text) (ext.issues + ext.merge_requests + ext.commits).uniq - [local_reference] @@ -83,7 +83,7 @@ module Mentionable # Only proceed if the saved changes actually include a chance to an attr_mentionable field. return unless mentionable_changed - preexisting = references(p, original) + preexisting = references(p, self.author, original) create_cross_references!(p, a, preexisting) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5634f9a686e..167d7f9c71f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -257,11 +257,11 @@ class MergeRequest < ActiveRecord::Base end # Return the set of issues that will be closed if this merge request is accepted. - def closes_issues + def closes_issues(current_user = self.author) if target_branch == project.default_branch - issues = commits.flat_map { |c| c.closes_issues(project) } + issues = commits.flat_map { |c| c.closes_issues(project, current_user) } issues.push(*Gitlab::ClosingIssueExtractor. - closed_by_message_in_project(description, project)) + closed_by_message_in_project(description, project, current_user)) issues.uniq.sort_by(&:id) else [] diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index bb066e39f6b..31e0167d247 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -70,7 +70,7 @@ class GitPushService # Close issues if these commits were pushed to the project's default branch and the commit message matches the # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to # a different branch. - issues_to_close = commit.closes_issues(project) + issues_to_close = commit.closes_issues(project, user) # Load commit author only if needed. # For push with 1k commits it prevents 900+ requests in database @@ -87,7 +87,7 @@ class GitPushService # Create cross-reference notes for any other references. Omit any issues that were referenced in an # issue-closing phrase, or have already been mentioned from this commit (probably from this commit # being pushed to a different branch). - refs = commit.references(project) - issues_to_close + refs = commit.references(project, user) - issues_to_close refs.reject! { |r| commit.has_mentioned?(r) } if refs.present? diff --git a/lib/gitlab/closing_issue_extractor.rb b/lib/gitlab/closing_issue_extractor.rb index 5643d60c807..6425193d85f 100644 --- a/lib/gitlab/closing_issue_extractor.rb +++ b/lib/gitlab/closing_issue_extractor.rb @@ -2,14 +2,14 @@ module Gitlab module ClosingIssueExtractor ISSUE_CLOSING_REGEX = Regexp.new(Gitlab.config.gitlab.issue_closing_pattern) - def self.closed_by_message_in_project(message, project) + def self.closed_by_message_in_project(message, project, current_user = nil) issues = [] unless message.nil? md = message.scan(ISSUE_CLOSING_REGEX) md.each do |ref| - extractor = Gitlab::ReferenceExtractor.new(project) + extractor = Gitlab::ReferenceExtractor.new(project, current_user) extractor.analyze(ref[0]) issues += extractor.issues end From 8361ba81abbab2d28ae5018daf531992710dc4ca Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 12:20:30 +0100 Subject: [PATCH 09/15] Add changelog item. --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 259e1a30072..28aac953627 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -52,6 +52,7 @@ v 7.10.0 (unreleased) - Don't show commit comment button when user is not signed in. - Fix admin user projects lists. - Don't leak private group existence by redirecting from namespace controller to group controller. + - Only allow users to reference groups, projects, issues, MRs, commits they have access to. v 7.9.2 - Contains no changes From c5d7660000be72dd03ac52641debbd2bcf6fbc4d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 27 Mar 2015 12:58:23 +0100 Subject: [PATCH 10/15] Fix errors. --- app/mailers/emails/groups.rb | 1 + app/mailers/emails/profile.rb | 6 +- app/mailers/emails/projects.rb | 5 +- app/mailers/notify.rb | 12 +- app/models/concerns/mentionable.rb | 2 +- app/services/projects/participants_service.rb | 19 +-- lib/gitlab/markdown.rb | 4 +- lib/gitlab/reference_extractor.rb | 24 +-- spec/helpers/gitlab_markdown_helper_spec.rb | 8 + spec/lib/gitlab/reference_extractor_spec.rb | 153 +++++++++--------- 10 files changed, 114 insertions(+), 120 deletions(-) diff --git a/app/mailers/emails/groups.rb b/app/mailers/emails/groups.rb index 26f43bf955e..626eb593d51 100644 --- a/app/mailers/emails/groups.rb +++ b/app/mailers/emails/groups.rb @@ -4,6 +4,7 @@ module Emails @group_member = GroupMember.find(group_member_id) @group = @group_member.group @target_url = group_url(@group) + @current_user = @group_member.user mail(to: @group_member.user.email, subject: subject("Access to group was granted")) end diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index ab5b0765352..3a83b083109 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -1,7 +1,7 @@ module Emails module Profile def new_user_email(user_id, token = nil) - @user = User.find(user_id) + @current_user = @user = User.find(user_id) @target_url = user_url(@user) @token = token mail(to: @user.notification_email, subject: subject("Account was created for you")) @@ -9,13 +9,13 @@ module Emails def new_email_email(email_id) @email = Email.find(email_id) - @user = @email.user + @current_user = @user = @email.user mail(to: @user.notification_email, subject: subject("Email was added to your account")) end def new_ssh_key_email(key_id) @key = Key.find(key_id) - @user = @key.user + @current_user = @user = @key.user @target_url = user_url(@user) mail(to: @user.notification_email, subject: subject("SSH key was added to your account")) end diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb index 3cd812825e2..20a863c3742 100644 --- a/app/mailers/emails/projects.rb +++ b/app/mailers/emails/projects.rb @@ -4,12 +4,13 @@ module Emails @project_member = ProjectMember.find user_project_id @project = @project_member.project @target_url = namespace_project_url(@project.namespace, @project) + @current_user = @project_member.user mail(to: @project_member.user.email, subject: subject("Access to project was granted")) end def project_was_moved_email(project_id, user_id) - @user = User.find user_id + @current_user = @user = User.find user_id @project = Project.find project_id @target_url = namespace_project_url(@project.namespace, @project) mail(to: @user.notification_email, @@ -28,7 +29,7 @@ module Emails end @project = Project.find(project_id) - @author = User.find(author_id) + @current_user = @author = User.find(author_id) @reverse_compare = reverse_compare @compare = compare @ref_name = Gitlab::Git.ref_name(ref) diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 8fcdd3bc853..5dbbfac9c8b 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -13,6 +13,9 @@ class Notify < ActionMailer::Base add_template_helper MergeRequestsHelper add_template_helper EmailsHelper + attr_accessor :current_user + helper_method :current_user, :can? + default_url_options[:host] = Gitlab.config.gitlab.host default_url_options[:protocol] = Gitlab.config.gitlab.protocol default_url_options[:port] = Gitlab.config.gitlab.port unless Gitlab.config.gitlab_on_standard_port? @@ -79,9 +82,8 @@ class Notify < ActionMailer::Base # # Returns a String containing the User's email address. def recipient(recipient_id) - if recipient = User.find(recipient_id) - recipient.notification_email - end + @current_user = User.find(recipient_id) + @current_user.notification_email end # Set the References header field @@ -154,4 +156,8 @@ class Notify < ActionMailer::Base mail(headers, &block) end + + def can? + Ability.abilities.allowed?(user, action, subject) + end end diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index bf0465728e7..b7882a2bb16 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -46,7 +46,7 @@ module Mentionable return [] if mentionable_text.blank? ext = Gitlab::ReferenceExtractor.new(self.project, current_user) - ext.analyze(text) + ext.analyze(mentionable_text) ext.users.uniq end diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb index bcdde0950c5..ae6260bcdab 100644 --- a/app/services/projects/participants_service.rb +++ b/app/services/projects/participants_service.rb @@ -1,10 +1,5 @@ module Projects class ParticipantsService < BaseService - def initialize(project, user) - @project = project - @user = user - end - def execute(note_type, note_id) participating = if note_type && note_id @@ -12,7 +7,7 @@ module Projects else [] end - project_members = sorted(@project.team.members) + project_members = sorted(project.team.members) participants = all_members + groups + project_members + participating participants.uniq end @@ -20,11 +15,11 @@ module Projects def participants_in(type, id) users = case type when "Issue" - issue = @project.issues.find_by_iid(id) - issue ? issue.participants(user) : [] + issue = project.issues.find_by_iid(id) + issue ? issue.participants(current_user) : [] when "MergeRequest" - merge_request = @project.merge_requests.find_by_iid(id) - merge_request ? merge_request.participants(user) : [] + merge_request = project.merge_requests.find_by_iid(id) + merge_request ? merge_request.participants(current_user) : [] when "Commit" author_ids = Note.for_commit_id(id).pluck(:author_id).uniq User.where(id: author_ids) @@ -41,14 +36,14 @@ module Projects end def groups - @user.authorized_groups.sort_by(&:path).map do |group| + current_user.authorized_groups.sort_by(&:path).map do |group| count = group.users.count { username: group.path, name: "#{group.name} (#{count})" } end end def all_members - count = @project.team.members.flatten.count + count = project.team.members.flatten.count [{ username: "all", name: "All Project and Group Members (#{count})" }] end end diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 7a3c8823e4d..e7c261b7453 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -192,7 +192,7 @@ module Gitlab project_path = $LAST_MATCH_INFO[:project] if project_path actual_project = ::Project.find_with_namespace(project_path) - actual_project ||= nil unless can?(user, :read_project, actual_project) + actual_project = nil unless can?(user, :read_project, actual_project) project_prefix = project_path end @@ -235,7 +235,7 @@ module Gitlab # # Returns string rendered by the processing method def reference_link(type, identifier, project = @project, user = current_user, prefix_text = nil) - send("reference_#{type}", identifier, project, prefix_text) + send("reference_#{type}", identifier, project, user, prefix_text) end def reference_user(identifier, project = @project, user = current_user, _ = nil) diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 719274394f0..2f38c1dcc89 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -7,7 +7,7 @@ module Gitlab def initialize(project, current_user = nil) @project = project - @current_user = user + @current_user = current_user @references = Hash.new { [] } end @@ -51,7 +51,7 @@ module Gitlab def issues references[:issues].map do |entry| - if should_lookup?(entry[:project]) + if entry[:project].default_issues_tracker? entry[:project].issues.where(iid: entry[:id]).first end end.compact @@ -59,9 +59,7 @@ module Gitlab def merge_requests references[:merge_requests].map do |entry| - if should_lookup?(entry[:project]) - entry[:project].merge_requests.where(iid: entry[:id]).first - end + entry[:project].merge_requests.where(iid: entry[:id]).first end.compact end @@ -73,17 +71,15 @@ module Gitlab def commits references[:commits].map do |entry| - repo = entry[:project].repository if entry[:project] - if should_lookup?(entry[:project]) - repo.commit(entry[:id]) if repo - end + repo = entry[:project].repository + repo.commit(entry[:id]) if repo end.compact end def commit_ranges references[:commit_ranges].map do |entry| repo = entry[:project].repository if entry[:project] - if repo && should_lookup?(entry[:project]) + if repo from_id, to_id = entry[:id].split(/\.{2,3}/, 2) [repo.commit(from_id), repo.commit(to_id)] end @@ -95,13 +91,5 @@ module Gitlab def reference_link(type, identifier, project, user, _) references[type] << { project: project, id: identifier } end - - def should_lookup?(entry_project) - if entry_project.nil? - false - else - project.nil? || entry_project.default_issues_tracker? - end - end end end diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 0d06c6ffb82..944e743675c 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -4,6 +4,11 @@ describe GitlabMarkdownHelper do include ApplicationHelper include IssuesHelper + # TODO: Properly test this + def can?(*) + true + end + let!(:project) { create(:project) } let(:empty_project) { create(:empty_project) } @@ -15,6 +20,9 @@ describe GitlabMarkdownHelper do let(:snippet) { create(:project_snippet, project: project) } let(:member) { project.project_members.where(user_id: user).first } + # Helper expects a current_user method. + let(:current_user) { user } + def url_helper(image_name) File.join(root_url, 'assets', image_name) end diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index b3f4bb5aeda..8ed4d5bc68f 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/spec/lib/gitlab/reference_extractor_spec.rb @@ -1,73 +1,76 @@ require 'spec_helper' describe Gitlab::ReferenceExtractor do + let(:project) { create(:project) } + subject { Gitlab::ReferenceExtractor.new(project) } + it 'extracts username references' do - subject.analyze('this contains a @user reference', nil) - expect(subject.users).to eq([{ project: nil, id: 'user' }]) + subject.analyze('this contains a @user reference') + expect(subject.users).to eq([{ project: project, id: 'user' }]) end it 'extracts issue references' do - subject.analyze('this one talks about issue #1234', nil) - expect(subject.issues).to eq([{ project: nil, id: '1234' }]) + subject.analyze('this one talks about issue #1234') + expect(subject.issues).to eq([{ project: project, id: '1234' }]) end it 'extracts JIRA issue references' do - subject.analyze('this one talks about issue JIRA-1234', nil) - expect(subject.issues).to eq([{ project: nil, id: 'JIRA-1234' }]) + subject.analyze('this one talks about issue JIRA-1234') + expect(subject.issues).to eq([{ project: project, id: 'JIRA-1234' }]) end it 'extracts merge request references' do - subject.analyze("and here's !43, a merge request", nil) - expect(subject.merge_requests).to eq([{ project: nil, id: '43' }]) + subject.analyze("and here's !43, a merge request") + expect(subject.merge_requests).to eq([{ project: project, id: '43' }]) end it 'extracts snippet ids' do - subject.analyze('snippets like $12 get extracted as well', nil) - expect(subject.snippets).to eq([{ project: nil, id: '12' }]) + subject.analyze('snippets like $12 get extracted as well') + expect(subject.snippets).to eq([{ project: project, id: '12' }]) end it 'extracts commit shas' do - subject.analyze('commit shas 98cf0ae3 are pulled out as Strings', nil) - expect(subject.commits).to eq([{ project: nil, id: '98cf0ae3' }]) + subject.analyze('commit shas 98cf0ae3 are pulled out as Strings') + expect(subject.commits).to eq([{ project: project, id: '98cf0ae3' }]) end it 'extracts commit ranges' do - subject.analyze('here you go, a commit range: 98cf0ae3...98cf0ae4', nil) - expect(subject.commit_ranges).to eq([{ project: nil, id: '98cf0ae3...98cf0ae4' }]) + subject.analyze('here you go, a commit range: 98cf0ae3...98cf0ae4') + expect(subject.commit_ranges).to eq([{ project: project, id: '98cf0ae3...98cf0ae4' }]) end it 'extracts multiple references and preserves their order' do - subject.analyze('@me and @you both care about this', nil) + subject.analyze('@me and @you both care about this') expect(subject.users).to eq([ - { project: nil, id: 'me' }, - { project: nil, id: 'you' } + { project: project, id: 'me' }, + { project: project, id: 'you' } ]) end it 'leaves the original note unmodified' do text = 'issue #123 is just the worst, @user' - subject.analyze(text, nil) + subject.analyze(text) expect(text).to eq('issue #123 is just the worst, @user') end it 'extracts no references for
..
blocks' do - subject.analyze("
def puts '#1 issue'\nend\n
```", nil) + subject.analyze("
def puts '#1 issue'\nend\n
```") expect(subject.issues).to be_blank end it 'extracts no references for .. blocks' do - subject.analyze("def puts '!1 request'\nend\n```", nil) + subject.analyze("def puts '!1 request'\nend\n```") expect(subject.merge_requests).to be_blank end it 'extracts no references for code blocks with language' do - subject.analyze("this code:\n```ruby\ndef puts '#1 issue'\nend\n```", nil) + subject.analyze("this code:\n```ruby\ndef puts '#1 issue'\nend\n```") expect(subject.issues).to be_blank end it 'extracts issue references for invalid code blocks' do - subject.analyze('test: ```this one talks about issue #1234```', nil) - expect(subject.issues).to eq([{ project: nil, id: '1234' }]) + subject.analyze('test: ```this one talks about issue #1234```') + expect(subject.issues).to eq([{ project: project, id: '1234' }]) end it 'handles all possible kinds of references' do @@ -75,70 +78,64 @@ describe Gitlab::ReferenceExtractor do expect(subject).to respond_to(*accessors) end - context 'with a project' do - let(:project) { create(:project) } + it 'accesses valid user objects on the project team' do + @u_foo = create(:user, username: 'foo') + @u_bar = create(:user, username: 'bar') + create(:user, username: 'offteam') - it 'accesses valid user objects on the project team' do - @u_foo = create(:user, username: 'foo') - @u_bar = create(:user, username: 'bar') - create(:user, username: 'offteam') + project.team << [@u_foo, :reporter] + project.team << [@u_bar, :guest] - project.team << [@u_foo, :reporter] - project.team << [@u_bar, :guest] + subject.analyze('@foo, @baduser, @bar, and @offteam') + expect(subject.users).to eq([@u_foo, @u_bar]) + end - subject.analyze('@foo, @baduser, @bar, and @offteam', project) - expect(subject.users_for(project)).to eq([@u_foo, @u_bar]) - end + it 'accesses valid issue objects' do + @i0 = create(:issue, project: project) + @i1 = create(:issue, project: project) - it 'accesses valid issue objects' do - @i0 = create(:issue, project: project) - @i1 = create(:issue, project: project) + subject.analyze("##{@i0.iid}, ##{@i1.iid}, and #999.") + expect(subject.issues).to eq([@i0, @i1]) + end - subject.analyze("##{@i0.iid}, ##{@i1.iid}, and #999.", project) - expect(subject.issues_for(project)).to eq([@i0, @i1]) - end + it 'accesses valid merge requests' do + @m0 = create(:merge_request, source_project: project, target_project: project, source_branch: 'aaa') + @m1 = create(:merge_request, source_project: project, target_project: project, source_branch: 'bbb') - it 'accesses valid merge requests' do - @m0 = create(:merge_request, source_project: project, target_project: project, source_branch: 'aaa') - @m1 = create(:merge_request, source_project: project, target_project: project, source_branch: 'bbb') + subject.analyze("!999, !#{@m1.iid}, and !#{@m0.iid}.") + expect(subject.merge_requests).to eq([@m1, @m0]) + end - subject.analyze("!999, !#{@m1.iid}, and !#{@m0.iid}.", project) - expect(subject.merge_requests_for(project)).to eq([@m1, @m0]) - end + it 'accesses valid snippets' do + @s0 = create(:project_snippet, project: project) + @s1 = create(:project_snippet, project: project) + @s2 = create(:project_snippet) - it 'accesses valid snippets' do - @s0 = create(:project_snippet, project: project) - @s1 = create(:project_snippet, project: project) - @s2 = create(:project_snippet) + subject.analyze("$#{@s0.id}, $999, $#{@s2.id}, $#{@s1.id}") + expect(subject.snippets).to eq([@s0, @s1]) + end - subject.analyze("$#{@s0.id}, $999, $#{@s2.id}, $#{@s1.id}", project) - expect(subject.snippets_for(project)).to eq([@s0, @s1]) - end + it 'accesses valid commits' do + commit = project.repository.commit('master') - it 'accesses valid commits' do - commit = project.repository.commit('master') + subject.analyze("this references commits #{commit.sha[0..6]} and 012345") + extracted = subject.commits + expect(extracted.size).to eq(1) + expect(extracted[0].sha).to eq(commit.sha) + expect(extracted[0].message).to eq(commit.message) + end - subject.analyze("this references commits #{commit.sha[0..6]} and 012345", - project) - extracted = subject.commits_for(project) - expect(extracted.size).to eq(1) - expect(extracted[0].sha).to eq(commit.sha) - expect(extracted[0].message).to eq(commit.message) - end + it 'accesses valid commit ranges' do + commit = project.repository.commit('master') + earlier_commit = project.repository.commit('master~2') - it 'accesses valid commit ranges' do - commit = project.repository.commit('master') - earlier_commit = project.repository.commit('master~2') - - subject.analyze("this references commits #{earlier_commit.sha[0..6]}...#{commit.sha[0..6]}", - project) - extracted = subject.commit_ranges_for(project) - expect(extracted.size).to eq(1) - expect(extracted[0][0].sha).to eq(earlier_commit.sha) - expect(extracted[0][0].message).to eq(earlier_commit.message) - expect(extracted[0][1].sha).to eq(commit.sha) - expect(extracted[0][1].message).to eq(commit.message) - end + subject.analyze("this references commits #{earlier_commit.sha[0..6]}...#{commit.sha[0..6]}") + extracted = subject.commit_ranges + expect(extracted.size).to eq(1) + expect(extracted[0][0].sha).to eq(earlier_commit.sha) + expect(extracted[0][0].message).to eq(earlier_commit.message) + expect(extracted[0][1].sha).to eq(commit.sha) + expect(extracted[0][1].message).to eq(commit.message) end context 'with a project with an underscore' do @@ -146,12 +143,10 @@ describe Gitlab::ReferenceExtractor do let(:issue) { create(:issue, project: project) } it 'handles project issue references' do - subject.analyze("this refers issue #{project.path_with_namespace}##{issue.iid}", - project) - extracted = subject.issues_for(project) + subject.analyze("this refers issue #{project.path_with_namespace}##{issue.iid}") + extracted = subject.issues expect(extracted.size).to eq(1) expect(extracted).to eq([issue]) end - end end From 7b9ae32eff09558cf55190b5aed32ad97b31bf55 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 3 Apr 2015 18:02:16 +0200 Subject: [PATCH 11/15] Allow byebug to be used in test. --- Gemfile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index e767aec5053..36838f52f40 100644 --- a/Gemfile +++ b/Gemfile @@ -208,7 +208,6 @@ group :development do gem "letter_opener" gem 'quiet_assets', '~> 1.0.1' gem 'rack-mini-profiler', require: false - gem "byebug" # Better errors handler gem 'better_errors' @@ -257,6 +256,8 @@ group :development, :test do gem "spring", '~> 1.3.1' gem "spring-commands-rspec", '1.0.4' gem "spring-commands-spinach", '1.0.0' + + gem "byebug" end group :test do From 9d647197da793b429102755e069686e6928de26e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 3 Apr 2015 18:03:01 +0200 Subject: [PATCH 12/15] Don't require user to every gfm call. --- app/helpers/gitlab_markdown_helper.rb | 2 +- lib/gitlab/markdown.rb | 42 +++++++++++++-------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index a4157d62533..7ca3f058636 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -19,7 +19,7 @@ module GitlabMarkdownHelper escape_once(body) end - gfm_body = gfm(escaped_body, @project, current_user, html_options) + gfm_body = gfm(escaped_body, @project, html_options) gfm_body.gsub!(%r{.*?}m) do |match| "#{match}#{link_to("", url, html_options)[0..-5]}" # "".length +1 diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index e7c261b7453..830de39a880 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -39,7 +39,7 @@ module Gitlab # text - the source text # project - the project # html_options - extra options for the reference links as given to link_to - def gfm(text, project = @project, user = current_user, html_options = {}) + def gfm(text, project = @project, html_options = {}) gfm_with_options(text, {}, project, html_options) end @@ -51,7 +51,7 @@ module Gitlab # - reference_only_path - Use relative path for reference links # project - the project # html_options - extra options for the reference links as given to link_to - def gfm_with_options(text, options = {}, project = @project, user = current_user, html_options = {}) + def gfm_with_options(text, options = {}, project = @project, html_options = {}) return text if text.nil? # Duplicate the string so we don't alter the original, then call to_str @@ -78,7 +78,7 @@ module Gitlab # TODO: add popups with additional information - text = parse(text, project, user) + text = parse(text, project) # Insert pre block extractions text.gsub!(/\{gfm-extraction-(\h{32})\}/) do @@ -155,8 +155,8 @@ module Gitlab # text - Text to parse # # Returns parsed text - def parse(text, project = @project, user = current_user) - parse_references(text, project, user) if project + def parse(text, project = @project) + parse_references(text, project) if project text end @@ -182,7 +182,7 @@ module Gitlab TYPES = [:user, :issue, :label, :merge_request, :snippet, :commit, :commit_range].freeze - def parse_references(text, project = @project, user = current_user) + def parse_references(text, project = @project) # parse reference links text.gsub!(REFERENCE_PATTERN) do |match| type = TYPES.select{|t| !$~[t].nil?}.first @@ -192,12 +192,12 @@ module Gitlab project_path = $LAST_MATCH_INFO[:project] if project_path actual_project = ::Project.find_with_namespace(project_path) - actual_project = nil unless can?(user, :read_project, actual_project) + actual_project = nil unless can?(current_user, :read_project, actual_project) project_prefix = project_path end parse_result($LAST_MATCH_INFO, type, - actual_project, user, project_prefix) || match + actual_project, project_prefix) || match end end @@ -205,7 +205,7 @@ module Gitlab # link. Returns nil if +type+ is nil, if the match string is an HTML # entity, if the reference is invalid, or if the matched text includes an # invalid project path. - def parse_result(match_info, type, project, user, project_prefix) + def parse_result(match_info, type, project, project_prefix) prefix = match_info[:prefix] suffix = match_info[:suffix] @@ -213,7 +213,7 @@ module Gitlab return nil if project.nil? && !project_prefix.nil? identifier = match_info[type] - ref_link = reference_link(type, identifier, project, user, project_prefix) + ref_link = reference_link(type, identifier, project, project_prefix) if ref_link "#{prefix}#{ref_link}#{suffix}" @@ -234,11 +234,11 @@ module Gitlab # identifier - Object identifier (Issue ID, SHA hash, etc.) # # Returns string rendered by the processing method - def reference_link(type, identifier, project = @project, user = current_user, prefix_text = nil) - send("reference_#{type}", identifier, project, user, prefix_text) + def reference_link(type, identifier, project = @project, prefix_text = nil) + send("reference_#{type}", identifier, project, prefix_text) end - def reference_user(identifier, project = @project, user = current_user, _ = nil) + def reference_user(identifier, project = @project, _ = nil) link_options = html_options.merge( class: "gfm gfm-project_member #{html_options[:class]}" ) @@ -252,7 +252,7 @@ module Gitlab elsif namespace = Namespace.find_by(path: identifier) url = if namespace.is_a?(Group) - return nil unless can?(user, :read_group, namespace) + return nil unless can?(current_user, :read_group, namespace) group_url(identifier, only_path: options[:reference_only_path]) else user_url(identifier, only_path: options[:reference_only_path]) @@ -262,7 +262,7 @@ module Gitlab end end - def reference_label(identifier, project = @project, user = current_user, _ = nil) + def reference_label(identifier, project = @project, _ = nil) if label = project.labels.find_by(id: identifier) link_options = html_options.merge( class: "gfm gfm-label #{html_options[:class]}" @@ -275,7 +275,7 @@ module Gitlab end end - def reference_issue(identifier, project = @project, user = current_user, prefix_text = nil) + def reference_issue(identifier, project = @project, prefix_text = nil) if project.default_issues_tracker? if project.issue_exists? identifier url = url_for_issue(identifier, project, only_path: options[:reference_only_path]) @@ -295,7 +295,7 @@ module Gitlab end end - def reference_merge_request(identifier, project = @project, user = current_user, prefix_text = nil) + def reference_merge_request(identifier, project = @project, prefix_text = nil) if merge_request = project.merge_requests.find_by(iid: identifier) link_options = html_options.merge( title: "Merge Request: #{merge_request.title}", @@ -308,7 +308,7 @@ module Gitlab end end - def reference_snippet(identifier, project = @project, user = current_user, _ = nil) + def reference_snippet(identifier, project = @project, _ = nil) if snippet = project.snippets.find_by(id: identifier) link_options = html_options.merge( title: "Snippet: #{snippet.title}", @@ -323,7 +323,7 @@ module Gitlab end end - def reference_commit(identifier, project = @project, user = current_user, prefix_text = nil) + def reference_commit(identifier, project = @project, prefix_text = nil) if project.valid_repo? && commit = project.repository.commit(identifier) link_options = html_options.merge( title: commit.link_title, @@ -339,7 +339,7 @@ module Gitlab end end - def reference_commit_range(identifier, project = @project, user = current_user, prefix_text = nil) + def reference_commit_range(identifier, project = @project, prefix_text = nil) from_id, to_id = identifier.split(/\.{2,3}/, 2) inclusive = identifier !~ /\.{3}/ @@ -365,7 +365,7 @@ module Gitlab end end - def reference_external_issue(identifier, project = @project, user = current_user, prefix_text = nil) + def reference_external_issue(identifier, project = @project, prefix_text = nil) url = url_for_issue(identifier, project, only_path: options[:reference_only_path]) title = project.external_issue_tracker.title From b492f0f86ea0f0b4e954e1b7ed0b84d08d784272 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 3 Apr 2015 18:03:15 +0200 Subject: [PATCH 13/15] Refactor ReferenceExtractor. --- lib/gitlab/reference_extractor.rb | 57 +++++++++++++++---------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 2f38c1dcc89..1a9a346d16d 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -8,8 +8,6 @@ module Gitlab def initialize(project, current_user = nil) @project = project @current_user = current_user - - @references = Hash.new { [] } end def can?(user, action, subject) @@ -23,6 +21,7 @@ module Gitlab text.gsub!(%r{
.*?
|.*?}m) { |match| '' } text.gsub!(%r{^```.*?^```}m) { |match| '' } + @references = Hash.new { |hash, type| hash[type] = [] } parse_references(text) end @@ -30,66 +29,66 @@ module Gitlab # model objects. def users - references[:users].map do |entry| - if entry[:id] == "all" + references[:user].uniq.map do |project, identifier| + if identifier == "all" project.team.members.flatten - elsif namespace = Namespace.find_by(path: entry[:id]) + elsif namespace = Namespace.find_by(path: identifier) if namespace.is_a?(Group) namespace.users else namespace.owner end end - end.flatten.compact + end.flatten.compact.uniq end def labels - references[:labels].map do |entry| - project.labels.where(id: entry[:id]).first - end.compact + references[:label].uniq.map do |project, identifier| + project.labels.where(id: identifier).first + end.compact.uniq end def issues - references[:issues].map do |entry| - if entry[:project].default_issues_tracker? - entry[:project].issues.where(iid: entry[:id]).first + references[:issue].uniq.map do |project, identifier| + if project.default_issues_tracker? + project.issues.where(iid: identifier).first end - end.compact + end.compact.uniq end def merge_requests - references[:merge_requests].map do |entry| - entry[:project].merge_requests.where(iid: entry[:id]).first - end.compact + references[:merge_request].uniq.map do |project, identifier| + project.merge_requests.where(iid: identifier).first + end.compact.uniq end def snippets - references[:snippets].map do |entry| - project.snippets.where(id: entry[:id]).first - end.compact + references[:snippet].uniq.map do |project, identifier| + project.snippets.where(id: identifier).first + end.compact.uniq end def commits - references[:commits].map do |entry| - repo = entry[:project].repository - repo.commit(entry[:id]) if repo - end.compact + references[:commit].uniq.map do |project, identifier| + repo = project.repository + repo.commit(identifier) if repo + end.compact.uniq end def commit_ranges - references[:commit_ranges].map do |entry| - repo = entry[:project].repository if entry[:project] + references[:commit_range].uniq.map do |project, identifier| + repo = project.repository if repo - from_id, to_id = entry[:id].split(/\.{2,3}/, 2) + from_id, to_id = identifier.split(/\.{2,3}/, 2) [repo.commit(from_id), repo.commit(to_id)] end - end.compact + end.compact.uniq end private - def reference_link(type, identifier, project, user, _) - references[type] << { project: project, id: identifier } + def reference_link(type, identifier, project, _) + references[type] << [project, identifier] end end end From e33ddfebf2248fe31bb27fe1d34048df97ed61b0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 3 Apr 2015 18:03:26 +0200 Subject: [PATCH 14/15] Refactor ClosingIssueExtractor. --- app/models/commit.rb | 2 +- app/models/merge_request.rb | 4 ++-- lib/gitlab/closing_issue_extractor.rb | 23 +++++++++++------------ 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index 084cf420079..7a0ad137650 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -118,7 +118,7 @@ class Commit # Discover issues should be closed when this commit is pushed to a project's # default branch. def closes_issues(project, current_user = self.committer) - Gitlab::ClosingIssueExtractor.closed_by_message_in_project(safe_message, project, current_user) + Gitlab::ClosingIssueExtractor.new(project, current_user).closed_by_message(safe_message) end # Mentionable override. diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 167d7f9c71f..35cb920d8bc 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -260,8 +260,8 @@ class MergeRequest < ActiveRecord::Base def closes_issues(current_user = self.author) if target_branch == project.default_branch issues = commits.flat_map { |c| c.closes_issues(project, current_user) } - issues.push(*Gitlab::ClosingIssueExtractor. - closed_by_message_in_project(description, project, current_user)) + issues.push(*Gitlab::ClosingIssueExtractor.new(project, current_user). + closed_by_message(description)) issues.uniq.sort_by(&:id) else [] diff --git a/lib/gitlab/closing_issue_extractor.rb b/lib/gitlab/closing_issue_extractor.rb index 6425193d85f..ab184d95c05 100644 --- a/lib/gitlab/closing_issue_extractor.rb +++ b/lib/gitlab/closing_issue_extractor.rb @@ -1,21 +1,20 @@ module Gitlab - module ClosingIssueExtractor + class ClosingIssueExtractor ISSUE_CLOSING_REGEX = Regexp.new(Gitlab.config.gitlab.issue_closing_pattern) - def self.closed_by_message_in_project(message, project, current_user = nil) - issues = [] + def initialize(project, current_user = nil) + @extractor = Gitlab::ReferenceExtractor.new(project, current_user) + end - unless message.nil? - md = message.scan(ISSUE_CLOSING_REGEX) + def closed_by_message(message) + return [] if message.nil? + + closing_statements = message.scan(ISSUE_CLOSING_REGEX). + map { |ref| ref[0] }.join(" ") - md.each do |ref| - extractor = Gitlab::ReferenceExtractor.new(project, current_user) - extractor.analyze(ref[0]) - issues += extractor.issues - end - end + @extractor.analyze(closing_statements) - issues.uniq + @extractor.issues end end end From 16e1076e6f69626e1d8bf53f52dc67baee9fb51e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 3 Apr 2015 18:03:34 +0200 Subject: [PATCH 15/15] Update tests. --- .../gitlab/closing_issue_extractor_spec.rb | 62 ++++++++++--------- spec/lib/gitlab/reference_extractor_spec.rb | 40 ++++++------ 2 files changed, 54 insertions(+), 48 deletions(-) diff --git a/spec/lib/gitlab/closing_issue_extractor_spec.rb b/spec/lib/gitlab/closing_issue_extractor_spec.rb index c96ee78e5fd..cb7b0fbb890 100644 --- a/spec/lib/gitlab/closing_issue_extractor_spec.rb +++ b/spec/lib/gitlab/closing_issue_extractor_spec.rb @@ -5,126 +5,128 @@ describe Gitlab::ClosingIssueExtractor do let(:issue) { create(:issue, project: project) } let(:iid1) { issue.iid } - describe :closed_by_message_in_project do + subject { described_class.new(project, project.creator) } + + describe "#closed_by_message" do context 'with a single reference' do it do message = "Awesome commit (Closes ##{iid1})" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Awesome commit (closes ##{iid1})" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Closed ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "closed ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Closing ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "closing ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Close ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "close ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Awesome commit (Fixes ##{iid1})" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Awesome commit (fixes ##{iid1})" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Fixed ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "fixed ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Fixing ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "fixing ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Fix ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "fix ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Awesome commit (Resolves ##{iid1})" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Awesome commit (resolves ##{iid1})" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Resolved ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "resolved ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Resolving ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "resolving ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "Resolve ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end it do message = "resolve ##{iid1}" - expect(subject.closed_by_message_in_project(message, project)).to eq([issue]) + expect(subject.closed_by_message(message)).to eq([issue]) end end @@ -137,28 +139,28 @@ describe Gitlab::ClosingIssueExtractor do it 'fetches issues in single line message' do message = "Closes ##{iid1} and fix ##{iid2}" - expect(subject.closed_by_message_in_project(message, project)). + expect(subject.closed_by_message(message)). to eq([issue, other_issue]) end it 'fetches comma-separated issues references in single line message' do message = "Closes ##{iid1}, closes ##{iid2}" - expect(subject.closed_by_message_in_project(message, project)). + expect(subject.closed_by_message(message)). to eq([issue, other_issue]) end it 'fetches comma-separated issues numbers in single line message' do message = "Closes ##{iid1}, ##{iid2} and ##{iid3}" - expect(subject.closed_by_message_in_project(message, project)). + expect(subject.closed_by_message(message)). to eq([issue, other_issue, third_issue]) end it 'fetches issues in multi-line message' do message = "Awesome commit (closes ##{iid1})\nAlso fixes ##{iid2}" - expect(subject.closed_by_message_in_project(message, project)). + expect(subject.closed_by_message(message)). to eq([issue, other_issue]) end @@ -166,7 +168,7 @@ describe Gitlab::ClosingIssueExtractor do message = "Awesome commit (closes ##{iid1})\n"\ "Also fixing issues ##{iid2}, ##{iid3} and #4" - expect(subject.closed_by_message_in_project(message, project)). + expect(subject.closed_by_message(message)). to eq([issue, other_issue, third_issue]) end end diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index 8ed4d5bc68f..c9fb62b61ae 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/spec/lib/gitlab/reference_extractor_spec.rb @@ -2,48 +2,48 @@ require 'spec_helper' describe Gitlab::ReferenceExtractor do let(:project) { create(:project) } - subject { Gitlab::ReferenceExtractor.new(project) } + subject { Gitlab::ReferenceExtractor.new(project, project.creator) } it 'extracts username references' do subject.analyze('this contains a @user reference') - expect(subject.users).to eq([{ project: project, id: 'user' }]) + expect(subject.references[:user]).to eq([[project, 'user']]) end it 'extracts issue references' do subject.analyze('this one talks about issue #1234') - expect(subject.issues).to eq([{ project: project, id: '1234' }]) + expect(subject.references[:issue]).to eq([[project, '1234']]) end it 'extracts JIRA issue references' do subject.analyze('this one talks about issue JIRA-1234') - expect(subject.issues).to eq([{ project: project, id: 'JIRA-1234' }]) + expect(subject.references[:issue]).to eq([[project, 'JIRA-1234']]) end it 'extracts merge request references' do subject.analyze("and here's !43, a merge request") - expect(subject.merge_requests).to eq([{ project: project, id: '43' }]) + expect(subject.references[:merge_request]).to eq([[project, '43']]) end it 'extracts snippet ids' do subject.analyze('snippets like $12 get extracted as well') - expect(subject.snippets).to eq([{ project: project, id: '12' }]) + expect(subject.references[:snippet]).to eq([[project, '12']]) end it 'extracts commit shas' do subject.analyze('commit shas 98cf0ae3 are pulled out as Strings') - expect(subject.commits).to eq([{ project: project, id: '98cf0ae3' }]) + expect(subject.references[:commit]).to eq([[project, '98cf0ae3']]) end it 'extracts commit ranges' do subject.analyze('here you go, a commit range: 98cf0ae3...98cf0ae4') - expect(subject.commit_ranges).to eq([{ project: project, id: '98cf0ae3...98cf0ae4' }]) + expect(subject.references[:commit_range]).to eq([[project, '98cf0ae3...98cf0ae4']]) end it 'extracts multiple references and preserves their order' do subject.analyze('@me and @you both care about this') - expect(subject.users).to eq([ - { project: project, id: 'me' }, - { project: project, id: 'you' } + expect(subject.references[:user]).to eq([ + [project, 'me'], + [project, 'you'] ]) end @@ -70,7 +70,7 @@ describe Gitlab::ReferenceExtractor do it 'extracts issue references for invalid code blocks' do subject.analyze('test: ```this one talks about issue #1234```') - expect(subject.issues).to eq([{ project: project, id: '1234' }]) + expect(subject.references[:issue]).to eq([[project, '1234']]) end it 'handles all possible kinds of references' do @@ -78,16 +78,16 @@ describe Gitlab::ReferenceExtractor do expect(subject).to respond_to(*accessors) end - it 'accesses valid user objects on the project team' do + it 'accesses valid user objects' do @u_foo = create(:user, username: 'foo') @u_bar = create(:user, username: 'bar') - create(:user, username: 'offteam') + @u_offteam = create(:user, username: 'offteam') project.team << [@u_foo, :reporter] project.team << [@u_bar, :guest] subject.analyze('@foo, @baduser, @bar, and @offteam') - expect(subject.users).to eq([@u_foo, @u_bar]) + expect(subject.users).to eq([@u_foo, @u_bar, @u_offteam]) end it 'accesses valid issue objects' do @@ -139,11 +139,15 @@ describe Gitlab::ReferenceExtractor do end context 'with a project with an underscore' do - let(:project) { create(:project, path: 'test_project') } - let(:issue) { create(:issue, project: project) } + let(:other_project) { create(:project, path: 'test_project') } + let(:issue) { create(:issue, project: other_project) } + + before do + other_project.team << [project.creator, :developer] + end it 'handles project issue references' do - subject.analyze("this refers issue #{project.path_with_namespace}##{issue.iid}") + subject.analyze("this refers issue #{other_project.path_with_namespace}##{issue.iid}") extracted = subject.issues expect(extracted.size).to eq(1) expect(extracted).to eq([issue])