diff options
| author | Douwe Maan <douwe@gitlab.com> | 2016-06-01 15:51:59 +0000 |
|---|---|---|
| committer | Douwe Maan <douwe@gitlab.com> | 2016-06-01 15:51:59 +0000 |
| commit | 2d084dd8481417ba8031a378d68009738d076229 (patch) | |
| tree | 6c91c3ce35a66bdd1fa1d7806da51968d564ad73 | |
| parent | ef6fe42ec05d40935154d2bb22a44fdb7d1eac8e (diff) | |
| parent | 580d250166d97bd5c2b0526be737d02806e577c2 (diff) | |
| download | gitlab-ce-2d084dd8481417ba8031a378d68009738d076229.tar.gz | |
Merge branch 'separate-banzai-references' into 'master'
Separate reference gathering from rendering
This is a required step to allow batch processing when gathering references. This in turn would allow grabbing (for example) all mentioned users of an issue/merge request using a single query.
cc @rspeicher @DouweM
See merge request !3969
93 files changed, 2133 insertions, 616 deletions
diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb index 0d6c32fabd2..4b404eb03fa 100644 --- a/app/controllers/projects/wikis_controller.rb +++ b/app/controllers/projects/wikis_controller.rb @@ -91,8 +91,8 @@ class Projects::WikisController < Projects::ApplicationController def markdown_preview text = params[:text] - ext = Gitlab::ReferenceExtractor.new(@project, current_user, current_user) - ext.analyze(text) + ext = Gitlab::ReferenceExtractor.new(@project, current_user) + ext.analyze(text, author: current_user) render json: { body: view_context.markdown(text, pipeline: :wiki, project_wiki: @project_wiki), diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 9697b88c032..f94e2a84fa2 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -197,8 +197,8 @@ class ProjectsController < Projects::ApplicationController def markdown_preview text = params[:text] - ext = Gitlab::ReferenceExtractor.new(@project, current_user, current_user) - ext.analyze(text) + ext = Gitlab::ReferenceExtractor.new(@project, current_user) + ext.analyze(text, author: current_user) render json: { body: view_context.markdown(text), diff --git a/app/models/ability.rb b/app/models/ability.rb index 8c5b255223d..44515550d9e 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -23,6 +23,28 @@ class Ability end.concat(global_abilities(user)) end + # Given a list of users and a project this method returns the users that can + # read the given project. + def users_that_can_read_project(users, project) + if project.public? + users + else + users.select do |user| + if user.admin? + true + elsif project.internal? && !user.external? + true + elsif project.owner == user + true + elsif project.team.members.include?(user) + true + else + false + end + end + end + end + # List of possible abilities for anonymous user def anonymous_abilities(user, subject) if subject.is_a?(PersonalSnippet) diff --git a/app/models/commit.rb b/app/models/commit.rb index 562c3ed15b2..f96c7cb34d0 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -8,7 +8,10 @@ class Commit include StaticModel attr_mentionable :safe_message, pipeline: :single_line - participant :author, :committer, :notes + + participant :author + participant :committer + participant :notes_with_associations attr_accessor :project @@ -194,6 +197,10 @@ class Commit project.notes.for_commit_id(self.id) end + def notes_with_associations + notes.includes(:author, :project) + end + def method_missing(m, *args, &block) @raw.send(m, *args, &block) end @@ -219,7 +226,7 @@ class Commit def revert_branch_name "revert-#{short_id}" end - + def cherry_pick_branch_name project.repository.next_branch("cherry-pick-#{short_id}", mild: true) end @@ -251,11 +258,13 @@ class Commit end def has_been_reverted?(current_user = nil, noteable = self) - Gitlab::ReferenceExtractor.lazily do - noteable.notes.system.flat_map do |note| - note.all_references(current_user).commits - end - end.any? { |commit_ref| commit_ref.reverts_commit?(self) } + ext = all_references(current_user) + + noteable.notes_with_associations.system.each do |note| + note.all_references(current_user, extractor: ext) + end + + ext.commits.any? { |commit_ref| commit_ref.reverts_commit?(self) } end def change_type_title diff --git a/app/models/commit_range.rb b/app/models/commit_range.rb index 51673897d98..4066958f67c 100644 --- a/app/models/commit_range.rb +++ b/app/models/commit_range.rb @@ -62,7 +62,7 @@ class CommitRange def initialize(range_string, project) @project = project - range_string.strip! + range_string = range_string.strip unless range_string =~ /\A#{PATTERN}\z/ raise ArgumentError, "invalid CommitRange string format: #{range_string}" diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 9aaaf787372..2326a395cb8 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -59,8 +59,12 @@ module Issuable prefix: true attr_mentionable :title, pipeline: :single_line - attr_mentionable :description, cache: true - participant :author, :assignee, :notes_with_associations + attr_mentionable :description + + participant :author + participant :assignee + participant :notes_with_associations + strip_attributes :title acts_as_paranoid diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index b381d225485..f00b5b8497c 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -23,7 +23,7 @@ module Mentionable included do if self < Participable - participant ->(current_user) { mentioned_users(current_user) } + participant -> (user, ext) { all_references(user, extractor: ext) } end end @@ -43,23 +43,22 @@ module Mentionable self end - def all_references(current_user = nil, text = nil) - ext = Gitlab::ReferenceExtractor.new(self.project, current_user || self.author, self.author) + def all_references(current_user = nil, text = nil, extractor: nil) + extractor ||= Gitlab::ReferenceExtractor. + new(project, current_user || author) if text - ext.analyze(text) + extractor.analyze(text, author: author) else self.class.mentionable_attrs.each do |attr, options| - text = send(attr) + text = __send__(attr) + options = options.merge(cache_key: [self, attr], author: author) - context = options.dup - context[:cache_key] = [self, attr] if context.delete(:cache) && self.persisted? - - ext.analyze(text, context) + extractor.analyze(text, options) end end - ext + extractor end def mentioned_users(current_user = nil) diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb index fc6f83b918b..9056722f45e 100644 --- a/app/models/concerns/participable.rb +++ b/app/models/concerns/participable.rb @@ -3,8 +3,6 @@ # Contains functionality related to objects that can have participants, such as # an author, an assignee and people mentioned in its description or comments. # -# Used by Issue, Note, MergeRequest, Snippet and Commit. -# # Usage: # # class Issue < ActiveRecord::Base @@ -12,22 +10,36 @@ # # # ... # -# participant :author, :assignee, :notes, ->(current_user) { mentioned_users(current_user) } +# participant :author +# participant :assignee +# participant :notes +# +# participant -> (current_user, ext) do +# ext.analyze('...') +# end # end # # issue = Issue.last # users = issue.participants -# # `users` will contain the issue's author, its assignee, -# # all users returned by its #mentioned_users method, -# # as well as all participants to all of the issue's notes, -# # since Note implements Participable as well. -# module Participable extend ActiveSupport::Concern module ClassMethods - def participant(*attrs) - participant_attrs.concat(attrs) + # Adds a list of participant attributes. Attributes can either be symbols or + # Procs. + # + # When using a Proc instead of a Symbol the Proc will be given two + # arguments: + # + # 1. The current user (as an instance of User) + # 2. An instance of `Gitlab::ReferenceExtractor` + # + # It is expected that a Proc populates the given reference extractor + # instance with data. The return value of the Proc is ignored. + # + # attr - The name of the attribute or a Proc + def participant(attr) + participant_attrs << attr end def participant_attrs @@ -35,42 +47,42 @@ module Participable end end - # 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) - participants = - Gitlab::ReferenceExtractor.lazily do - self.class.participant_attrs.flat_map do |attr| - value = - if attr.respond_to?(:call) - instance_exec(current_user, &attr) - else - send(attr) - end + # Returns the users participating in a discussion. + # + # This method processes attributes of objects in breadth-first order. + # + # Returns an Array of User instances. + def participants(current_user = nil) + current_user ||= author + ext = Gitlab::ReferenceExtractor.new(project, current_user) + participants = Set.new + process = [self] - participants_for(value, current_user) - end.compact.uniq - end + until process.empty? + source = process.pop - unless Gitlab::ReferenceExtractor.lazy? - participants.select! do |user| - user.can?(:read_project, project) + case source + when User + participants << source + when Participable + source.class.participant_attrs.each do |attr| + if attr.respond_to?(:call) + source.instance_exec(current_user, ext, &attr) + else + process << source.__send__(attr) + end + end + when Enumerable, ActiveRecord::Relation + # This uses reverse_each so we can use "pop" to get the next value to + # process (in order). Using unshift instead of pop would require + # moving all Array values one index to the left (which can be + # expensive). + source.reverse_each { |obj| process << obj } end end - participants - end - - private + participants.merge(ext.users) - def participants_for(value, current_user = nil) - case value - when User, Banzai::LazyReference - [value] - when Enumerable, ActiveRecord::Relation - value.flat_map { |v| participants_for(v, current_user) } - when Participable - value.participants(current_user) - end + Ability.users_that_can_read_project(participants.to_a, project) end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 2d4a9b9f19a..bd0fbc96d18 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -95,14 +95,13 @@ class Issue < ActiveRecord::Base end def referenced_merge_requests(current_user = nil) - @referenced_merge_requests ||= {} - @referenced_merge_requests[current_user] ||= begin - Gitlab::ReferenceExtractor.lazily do - [self, *notes].flat_map do |note| - note.all_references(current_user).merge_requests - end - end.sort_by(&:iid).uniq + ext = all_references(current_user) + + notes_with_associations.each do |object| + object.all_references(current_user, extractor: ext) end + + ext.merge_requests.sort_by(&:iid) end # All branches containing the current issue's ID, except for @@ -139,9 +138,13 @@ class Issue < ActiveRecord::Base def closed_by_merge_requests(current_user = nil) return [] unless open? - notes.system.flat_map do |note| - note.all_references(current_user).merge_requests - end.uniq.select { |mr| mr.open? && mr.closes_issue?(self) } + ext = all_references(current_user) + + notes.system.each do |note| + note.all_references(current_user, extractor: ext) + end + + ext.merge_requests.select { |mr| mr.open? && mr.closes_issue?(self) } end def moved? diff --git a/app/models/note.rb b/app/models/note.rb index 052239af43c..c21981ead84 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -6,7 +6,7 @@ class Note < ActiveRecord::Base default_value_for :system, false - attr_mentionable :note, cache: true, pipeline: :note + attr_mentionable :note, pipeline: :note participant :author belongs_to :project diff --git a/app/models/project_snippet.rb b/app/models/project_snippet.rb index 5fba6baa204..25b5d777641 100644 --- a/app/models/project_snippet.rb +++ b/app/models/project_snippet.rb @@ -7,5 +7,6 @@ class ProjectSnippet < Snippet # Scopes scope :fresh, -> { order("created_at DESC") } - participant :author, :notes + participant :author + participant :notes_with_associations end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 0a3c3b57669..407697b745c 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -30,7 +30,8 @@ class Snippet < ActiveRecord::Base scope :public_and_internal, -> { where(visibility_level: [Snippet::PUBLIC, Snippet::INTERNAL]) } scope :fresh, -> { order("created_at DESC") } - participant :author, :notes + participant :author + participant :notes_with_associations def self.reference_prefix '$' @@ -100,6 +101,10 @@ class Snippet < ActiveRecord::Base content.lines.count > 1000 end + def notes_with_associations + notes.includes(:author, :project) + end + class << self # Searches for snippets with a matching title or file name. # diff --git a/app/views/admin/abuse_reports/_abuse_report.html.haml b/app/views/admin/abuse_reports/_abuse_report.html.haml index 2ab01704b77..862b86d9d4a 100644 --- a/app/views/admin/abuse_reports/_abuse_report.html.haml +++ b/app/views/admin/abuse_reports/_abuse_report.html.haml @@ -16,7 +16,7 @@ .light.small = time_ago_with_tooltip(abuse_report.created_at) %td - = markdown(abuse_report.message.squish!, pipeline: :single_line) + = markdown(abuse_report.message.squish!, pipeline: :single_line, author: reporter) %td - if user = link_to 'Remove user & report', admin_abuse_report_path(abuse_report, remove_user: true), diff --git a/app/views/events/_commit.html.haml b/app/views/events/_commit.html.haml index dce4081288c..1bc9f604438 100644 --- a/app/views/events/_commit.html.haml +++ b/app/views/events/_commit.html.haml @@ -2,4 +2,4 @@ .commit-row-title = link_to truncate_sha(commit[:id]), namespace_project_commit_path(project.namespace, project, commit[:id]), class: "commit_short_id", alt: '', title: truncate_sha(commit[:id]) · - = markdown event_commit_title(commit[:message]), project: project, pipeline: :single_line + = markdown event_commit_title(commit[:message]), project: project, pipeline: :single_line, author: event.author diff --git a/app/views/events/_event_issue.atom.haml b/app/views/events/_event_issue.atom.haml index fad65310021..083c3936212 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, pipeline: :atom, project: issue.project) + = markdown(issue.description, pipeline: :atom, project: issue.project, author: issue.author) diff --git a/app/views/events/_event_merge_request.atom.haml b/app/views/events/_event_merge_request.atom.haml index 19bdc7b9ca5..d7e05600627 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, pipeline: :atom, project: merge_request.project) + = markdown(merge_request.description, pipeline: :atom, project: merge_request.project, author: merge_request.author) diff --git a/app/views/events/_event_note.atom.haml b/app/views/events/_event_note.atom.haml index b730ebbd5f9..1154f982821 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, pipeline: :atom, project: note.project) + = markdown(note.note, pipeline: :atom, project: note.project, author: note.author) diff --git a/app/views/events/_event_push.atom.haml b/app/views/events/_event_push.atom.haml index b271b9daff1..28bee1d0a33 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]), pipeline: :atom, project: event.project) + %blockquote= markdown(escape_once(commit[:message]), pipeline: :atom, project: event.project, author: event.author) - if event.commits_count > 15 %p %i diff --git a/app/views/events/event/_push.html.haml b/app/views/events/event/_push.html.haml index 235bd46107e..dc4ff17e31a 100644 --- a/app/views/events/event/_push.html.haml +++ b/app/views/events/event/_push.html.haml @@ -15,7 +15,7 @@ %ul.well-list.event_commits - few_commits = event.commits[0...2] - few_commits.each do |commit| - = render "events/commit", commit: commit, project: project + = render "events/commit", commit: commit, project: project, event: event - create_mr = event.new_ref? && create_mr_button?(event.project.default_branch, event.ref_name, event.project) - if event.commits_count > 1 diff --git a/app/views/notify/_note_message.html.haml b/app/views/notify/_note_message.html.haml index 12ded41fbf2..e9c66170877 100644 --- a/app/views/notify/_note_message.html.haml +++ b/app/views/notify/_note_message.html.haml @@ -2,4 +2,4 @@ %div #{link_to @note.author_name, user_url(@note.author)} wrote: %div - = markdown(@note.note, pipeline: :email) + = markdown(@note.note, pipeline: :email, author: @note.author) diff --git a/app/views/notify/new_issue_email.html.haml b/app/views/notify/new_issue_email.html.haml index ad3ab2525bb..f42b150c0d6 100644 --- a/app/views/notify/new_issue_email.html.haml +++ b/app/views/notify/new_issue_email.html.haml @@ -2,7 +2,7 @@ %div #{link_to @issue.author_name, user_url(@issue.author)} wrote: -if @issue.description - = markdown(@issue.description, pipeline: :email) + = markdown(@issue.description, pipeline: :email, author: @issue.author) - 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 23423e7d981..158404de396 100644 --- a/app/views/notify/new_merge_request_email.html.haml +++ b/app/views/notify/new_merge_request_email.html.haml @@ -9,4 +9,4 @@ Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name} -if @merge_request.description - = markdown(@merge_request.description, pipeline: :email) + = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author) diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml index 429bf041248..6674d58417b 100644 --- a/app/views/projects/commit/_commit_box.html.haml +++ b/app/views/projects/commit/_commit_box.html.haml @@ -63,10 +63,10 @@ .commit-box.content-block %h3.commit-title - = markdown escape_once(@commit.title), pipeline: :single_line + = markdown escape_once(@commit.title), pipeline: :single_line, author: @commit.author - if @commit.description.present? %pre.commit-description - = preserve(markdown(escape_once(@commit.description), pipeline: :single_line)) + = preserve(markdown(escape_once(@commit.description), pipeline: :single_line, author: @commit.author)) :javascript $(".commit-info.branches").load("#{branches_namespace_project_commit_path(@project.namespace, @project, @commit.id)}"); diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 655cb0ac3cb..367027182b6 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -24,7 +24,7 @@ - if commit.description? .commit-row-description.js-toggle-content %pre - = preserve(markdown(escape_once(commit.description), pipeline: :single_line)) + = preserve(markdown(escape_once(commit.description), pipeline: :single_line, author: commit.author)) .commit-row-info by diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index e95f42064ac..f3b0469b7d4 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -52,12 +52,12 @@ .issue-details.issuable-details .detail-page-description.content-block %h2.title - = markdown escape_once(@issue.title), pipeline: :single_line + = markdown escape_once(@issue.title), pipeline: :single_line, author: @issue.author - if @issue.description.present? .description{ class: can?(current_user, :update_issue, @issue) ? 'js-task-list-container' : '' } .wiki = preserve do - = markdown(@issue.description, cache_key: [@issue, "description"]) + = markdown(@issue.description, cache_key: [@issue, "description"], author: @issue.author) %textarea.hidden.js-task-list-field = @issue.description = edited_time_ago_with_tooltip(@issue, placement: 'bottom', html_class: 'issue_edited_ago') diff --git a/app/views/projects/merge_requests/show/_mr_box.html.haml b/app/views/projects/merge_requests/show/_mr_box.html.haml index a23bd8d18d0..ebf18f6ac85 100644 --- a/app/views/projects/merge_requests/show/_mr_box.html.haml +++ b/app/views/projects/merge_requests/show/_mr_box.html.haml @@ -1,13 +1,13 @@ .detail-page-description.content-block %h2.title - = markdown escape_once(@merge_request.title), pipeline: :single_line + = markdown escape_once(@merge_request.title), pipeline: :single_line, author: @merge_request.author %div - if @merge_request.description.present? .description{class: can?(current_user, :update_merge_request, @merge_request) ? 'js-task-list-container' : ''} .wiki = preserve do - = markdown(@merge_request.description, cache_key: [@merge_request, "description"]) + = markdown(@merge_request.description, cache_key: [@merge_request, "description"], author: @merge_request.author) %textarea.hidden.js-task-list-field = @merge_request.description diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index 55dbae598d3..13359abede7 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/app/views/projects/merge_requests/widget/_open.html.haml @@ -26,4 +26,4 @@ %i.fa.fa-check Accepting this merge request will close #{"issue".pluralize(@closes_issues.size)} = succeed '.' do - != markdown issues_sentence(@closes_issues), pipeline: :gfm + != markdown issues_sentence(@closes_issues), pipeline: :gfm, author: @merge_request.author diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index 9fbc9a45549..f1045bbd8c3 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -29,7 +29,7 @@ .note-body{class: note_editable ? 'js-task-list-container' : ''} .note-text = preserve do - = markdown(note.note, pipeline: :note, cache_key: [note, "note"]) + = markdown(note.note, pipeline: :note, cache_key: [note, "note"], author: note.author) - if note_editable = render 'projects/notes/edit_form', note: note = edited_time_ago_with_tooltip(note, placement: 'bottom', html_class: 'note_edited_ago', include_author: true) diff --git a/app/views/projects/repositories/_feed.html.haml b/app/views/projects/repositories/_feed.html.haml index 6ca919f7f80..43a6fdfd103 100644 --- a/app/views/projects/repositories/_feed.html.haml +++ b/app/views/projects/repositories/_feed.html.haml @@ -12,7 +12,7 @@ = link_to namespace_project_commits_path(@project.namespace, @project, commit.id) do %code= commit.short_id = image_tag avatar_icon(commit.author_email), class: "", width: 16, alt: '' - = markdown escape_once(truncate(commit.title, length: 40)), pipeline: :single_line + = markdown escape_once(truncate(commit.title, length: 40)), pipeline: :single_line, author: commit.author %td %span.pull-right.cgray = time_ago_with_tooltip(commit.committed_date) diff --git a/app/views/search/results/_issue.html.haml b/app/views/search/results/_issue.html.haml index 640890fbe92..8f68d6d1b87 100644 --- a/app/views/search/results/_issue.html.haml +++ b/app/views/search/results/_issue.html.haml @@ -7,7 +7,7 @@ - if issue.description.present? .description.term = preserve do - = search_md_sanitize(markdown(truncate(issue.description, length: 200, separator: " "), { project: issue.project })) + = search_md_sanitize(markdown(truncate(issue.description, length: 200, separator: " "), { project: issue.project, author: issue.author })) %span.light #{issue.project.name_with_namespace} - if issue.closed? diff --git a/app/views/search/results/_merge_request.html.haml b/app/views/search/results/_merge_request.html.haml index 333f6533213..6331c2bd6b0 100644 --- a/app/views/search/results/_merge_request.html.haml +++ b/app/views/search/results/_merge_request.html.haml @@ -6,7 +6,7 @@ - if merge_request.description.present? .description.term = preserve do - = search_md_sanitize(markdown(merge_request.description, { project: merge_request.project })) + = search_md_sanitize(markdown(merge_request.description, { project: merge_request.project, author: merge_request.author })) %span.light #{merge_request.project.name_with_namespace} .pull-right diff --git a/app/views/search/results/_note.html.haml b/app/views/search/results/_note.html.haml index d9400b1d9fa..8163aff43b6 100644 --- a/app/views/search/results/_note.html.haml +++ b/app/views/search/results/_note.html.haml @@ -19,4 +19,4 @@ .note-search-result .term = preserve do - = search_md_sanitize(markdown(note.note, {no_header_anchors: true})) + = search_md_sanitize(markdown(note.note, {no_header_anchors: true, author: note.author})) diff --git a/app/views/shared/snippets/_header.html.haml b/app/views/shared/snippets/_header.html.haml index 8d391d85642..af753496260 100644 --- a/app/views/shared/snippets/_header.html.haml +++ b/app/views/shared/snippets/_header.html.haml @@ -21,4 +21,4 @@ .content-block.second-block %h2.snippet-title.prepend-top-0.append-bottom-0 - = markdown escape_once(@snippet.title), pipeline: :single_line + = markdown escape_once(@snippet.title), pipeline: :single_line, author: @snippet.author diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index 65283fe96a4..db95d7c908b 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -18,10 +18,6 @@ module Banzai @object_sym ||= object_name.to_sym end - def self.data_reference - @data_reference ||= "data-#{object_name.dasherize}" - end - def self.object_class_title @object_title ||= object_class.name.titleize end @@ -45,10 +41,6 @@ module Banzai end end - def self.referenced_by(node) - { object_sym => LazyReference.new(object_class, node.attr(data_reference)) } - end - def object_class self.class.object_class end diff --git a/lib/banzai/filter/commit_range_reference_filter.rb b/lib/banzai/filter/commit_range_reference_filter.rb index b469ea0f626..bbb88c979cc 100644 --- a/lib/banzai/filter/commit_range_reference_filter.rb +++ b/lib/banzai/filter/commit_range_reference_filter.rb @@ -4,6 +4,8 @@ module Banzai # # This filter supports cross-project references. class CommitRangeReferenceFilter < AbstractReferenceFilter + self.reference_type = :commit_range + def self.object_class CommitRange end @@ -14,34 +16,18 @@ module Banzai 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 = find_object(project, id) - - return unless range - - { commit_range: range } - end - def initialize(*args) super @commit_map = {} end - def self.find_object(project, id) + def find_object(project, id) range = CommitRange.new(id, project) range.valid_commits? ? range : nil end - def find_object(*args) - self.class.find_object(*args) - end - def url_for_object(range, project) h = Gitlab::Routing.url_helpers h.namespace_project_compare_url(project.namespace, project, diff --git a/lib/banzai/filter/commit_reference_filter.rb b/lib/banzai/filter/commit_reference_filter.rb index bd88207326c..2ce1816672b 100644 --- a/lib/banzai/filter/commit_reference_filter.rb +++ b/lib/banzai/filter/commit_reference_filter.rb @@ -4,6 +4,8 @@ module Banzai # # This filter supports cross-project references. class CommitReferenceFilter < AbstractReferenceFilter + self.reference_type = :commit + def self.object_class Commit end @@ -14,28 +16,12 @@ module Banzai 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 = find_object(project, id) - - return unless commit - - { commit: commit } - end - - def self.find_object(project, id) + def find_object(project, id) if project && project.valid_repo? project.commit(id) end end - def find_object(*args) - self.class.find_object(*args) - end - def url_for_object(commit, project) h = Gitlab::Routing.url_helpers h.namespace_project_commit_url(project.namespace, project, commit, diff --git a/lib/banzai/filter/external_issue_reference_filter.rb b/lib/banzai/filter/external_issue_reference_filter.rb index 37344b90576..eaa702952cc 100644 --- a/lib/banzai/filter/external_issue_reference_filter.rb +++ b/lib/banzai/filter/external_issue_reference_filter.rb @@ -4,6 +4,8 @@ module Banzai # References are ignored if the project doesn't use an external issue # tracker. class ExternalIssueReferenceFilter < ReferenceFilter + self.reference_type = :external_issue + # Public: Find `JIRA-123` issue references in text # # ExternalIssueReferenceFilter.references_in(text) do |match, issue| @@ -21,18 +23,6 @@ module Banzai end end - def self.referenced_by(node) - project = Project.find(node.attr("data-project")) rescue nil - return unless project - - id = node.attr("data-external-issue") - external_issue = ExternalIssue.new(id, project) - - return unless external_issue - - { external_issue: external_issue } - end - def call # Early return if the project isn't using an external tracker return doc if project.nil? || default_issues_tracker? diff --git a/lib/banzai/filter/issue_reference_filter.rb b/lib/banzai/filter/issue_reference_filter.rb index 59c5e89c546..2496e704002 100644 --- a/lib/banzai/filter/issue_reference_filter.rb +++ b/lib/banzai/filter/issue_reference_filter.rb @@ -5,18 +5,12 @@ module Banzai # # This filter supports cross-project references. class IssueReferenceFilter < AbstractReferenceFilter + self.reference_type = :issue + def self.object_class Issue end - def self.user_can_see_reference?(user, node, context) - # It is not possible to check access rights for external issue trackers - return true if context[:project].try(:external_issue_tracker) - - issue = Issue.find(node.attr('data-issue')) rescue nil - Ability.abilities.allowed?(user, :read_issue, issue) - end - def find_object(project, id) project.get_issue(id) end diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb index 8488a493b55..e4d3f87d0aa 100644 --- a/lib/banzai/filter/label_reference_filter.rb +++ b/lib/banzai/filter/label_reference_filter.rb @@ -2,6 +2,8 @@ module Banzai module Filter # HTML filter that replaces label references with links. class LabelReferenceFilter < AbstractReferenceFilter + self.reference_type = :label + def self.object_class Label end diff --git a/lib/banzai/filter/merge_request_reference_filter.rb b/lib/banzai/filter/merge_request_reference_filter.rb index cad38a51851..ac5216d9cfb 100644 --- a/lib/banzai/filter/merge_request_reference_filter.rb +++ b/lib/banzai/filter/merge_request_reference_filter.rb @@ -5,6 +5,8 @@ module Banzai # # This filter supports cross-project references. class MergeRequestReferenceFilter < AbstractReferenceFilter + self.reference_type = :merge_request + def self.object_class MergeRequest end diff --git a/lib/banzai/filter/milestone_reference_filter.rb b/lib/banzai/filter/milestone_reference_filter.rb index dad0768f51b..ca686c87d97 100644 --- a/lib/banzai/filter/milestone_reference_filter.rb +++ b/lib/banzai/filter/milestone_reference_filter.rb @@ -2,6 +2,8 @@ module Banzai module Filter # HTML filter that replaces milestone references with links. class MilestoneReferenceFilter < AbstractReferenceFilter + self.reference_type = :milestone + def self.object_class Milestone end diff --git a/lib/banzai/filter/redactor_filter.rb b/lib/banzai/filter/redactor_filter.rb index e589b5df6ec..c753a84a20d 100644 --- a/lib/banzai/filter/redactor_filter.rb +++ b/lib/banzai/filter/redactor_filter.rb @@ -7,8 +7,11 @@ module Banzai # class RedactorFilter < HTML::Pipeline::Filter def call - Querying.css(doc, 'a.gfm').each do |node| - unless user_can_see_reference?(node) + nodes = Querying.css(doc, 'a.gfm[data-reference-type]') + visible = nodes_visible_to_user(nodes) + + nodes.each do |node| + unless visible.include?(node) # The reference should be replaced by the original text, # which is not always the same as the rendered text. text = node.attr('data-original') || node.text @@ -21,20 +24,30 @@ module Banzai private - def user_can_see_reference?(node) - if node.has_attribute?('data-reference-filter') - reference_type = node.attr('data-reference-filter') - reference_filter = Banzai::Filter.const_get(reference_type) + def nodes_visible_to_user(nodes) + per_type = Hash.new { |h, k| h[k] = [] } + visible = Set.new + + nodes.each do |node| + per_type[node.attr('data-reference-type')] << node + end + + per_type.each do |type, nodes| + parser = Banzai::ReferenceParser[type].new(project, current_user) - reference_filter.user_can_see_reference?(current_user, node, context) - else - true + visible.merge(parser.nodes_visible_to_user(current_user, nodes)) end + + visible end def current_user context[:current_user] end + + def project + context[:project] + end end end end diff --git a/lib/banzai/filter/reference_filter.rb b/lib/banzai/filter/reference_filter.rb index 31386cf851c..41ae0e1f9cc 100644 --- a/lib/banzai/filter/reference_filter.rb +++ b/lib/banzai/filter/reference_filter.rb @@ -8,24 +8,8 @@ module Banzai # :project (required) - Current project, ignored if reference is cross-project. # :only_path - Generate path-only links. class ReferenceFilter < HTML::Pipeline::Filter - def self.user_can_see_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].try(:id) - - project = Project.find(project_id) rescue nil - Ability.abilities.allowed?(user, :read_project, project) - else - true - end - end - - def self.user_can_reference?(user, node, context) - true - end - - def self.referenced_by(node) - raise NotImplementedError, "#{self} does not implement #{__method__}" + class << self + attr_accessor :reference_type end # Returns a data attribute String to attach to a reference link @@ -43,7 +27,9 @@ module Banzai # # Returns a String def data_attribute(attributes = {}) - attributes[:reference_filter] = self.class.name.demodulize + attributes = attributes.reject { |_, v| v.nil? } + + attributes[:reference_type] = self.class.reference_type attributes.delete(:original) if context[:no_original_data] attributes.map { |key, value| %Q(data-#{key.to_s.dasherize}="#{escape_once(value)}") }.join(" ") end diff --git a/lib/banzai/filter/reference_gatherer_filter.rb b/lib/banzai/filter/reference_gatherer_filter.rb deleted file mode 100644 index 96fdb06304e..00000000000 --- a/lib/banzai/filter/reference_gatherer_filter.rb +++ /dev/null @@ -1,65 +0,0 @@ -module Banzai - module Filter - # 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. - # - class ReferenceGathererFilter < HTML::Pipeline::Filter - def initialize(*) - super - - result[:references] ||= Hash.new { |hash, type| hash[type] = [] } - end - - def call - Querying.css(doc, 'a.gfm').each do |node| - gather_references(node) - end - - load_lazy_references unless ReferenceExtractor.lazy? - - doc - end - - private - - def gather_references(node) - return unless node.has_attribute?('data-reference-filter') - - reference_type = node.attr('data-reference-filter') - reference_filter = Banzai::Filter.const_get(reference_type) - - return if context[:reference_filter] && reference_filter != context[:reference_filter] - - return if author && !reference_filter.user_can_reference?(author, node, context) - - return unless reference_filter.user_can_see_reference?(current_user, node, context) - - references = reference_filter.referenced_by(node) - return unless references - - references.each do |type, values| - Array.wrap(values).each do |value| - result[:references][type] << value - end - end - end - - def load_lazy_references - refs = result[:references] - refs.each do |type, values| - refs[type] = ReferenceExtractor.lazily(values) - end - end - - def current_user - context[:current_user] - end - - def author - context[:author] - end - end - end -end diff --git a/lib/banzai/filter/snippet_reference_filter.rb b/lib/banzai/filter/snippet_reference_filter.rb index d507eb5ebe1..212a0bbf2a0 100644 --- a/lib/banzai/filter/snippet_reference_filter.rb +++ b/lib/banzai/filter/snippet_reference_filter.rb @@ -5,6 +5,8 @@ module Banzai # # This filter supports cross-project references. class SnippetReferenceFilter < AbstractReferenceFilter + self.reference_type = :snippet + def self.object_class Snippet end diff --git a/lib/banzai/filter/user_reference_filter.rb b/lib/banzai/filter/user_reference_filter.rb index eea3af842b6..331d8007257 100644 --- a/lib/banzai/filter/user_reference_filter.rb +++ b/lib/banzai/filter/user_reference_filter.rb @@ -4,6 +4,8 @@ module Banzai # # A special `@all` reference is also supported. class UserReferenceFilter < ReferenceFilter + self.reference_type = :user + # Public: Find `@user` user references in text # # UserReferenceFilter.references_in(text) do |match, username| @@ -21,43 +23,6 @@ module Banzai 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: 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 - - { user: project.team.members.flatten } - end - end - - def self.user_can_see_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) - else - super - end - end - - def self.user_can_reference?(user, node, context) - # Only team members can reference `@all` - if node.has_attribute?('data-project') - project = Project.find(node.attr('data-project')) rescue nil - return false unless project - - user && project.team.member?(user) - else - super - end - end - def call return doc if project.nil? @@ -114,9 +79,12 @@ module Banzai def link_to_all(link_text: nil) project = context[:project] + author = context[:author] + url = urls.namespace_project_url(project.namespace, project, only_path: context[:only_path]) - data = data_attribute(project: project.id) + + data = data_attribute(project: project.id, author: author.try(:id)) text = link_text || User.reference_prefix + 'all' link_tag(url, data, text) diff --git a/lib/banzai/lazy_reference.rb b/lib/banzai/lazy_reference.rb deleted file mode 100644 index 1095b4debc7..00000000000 --- a/lib/banzai/lazy_reference.rb +++ /dev/null @@ -1,25 +0,0 @@ -module Banzai - class LazyReference - def self.load(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 - - attr_reader :klass, :ids - - def initialize(klass, ids) - @klass = klass - @ids = Array.wrap(ids).map(&:to_i) - end - - def load - self.klass.where(id: self.ids) - end - end -end diff --git a/lib/banzai/pipeline/reference_extraction_pipeline.rb b/lib/banzai/pipeline/reference_extraction_pipeline.rb deleted file mode 100644 index 919998380e4..00000000000 --- a/lib/banzai/pipeline/reference_extraction_pipeline.rb +++ /dev/null @@ -1,11 +0,0 @@ -module Banzai - module Pipeline - class ReferenceExtractionPipeline < BasePipeline - def self.filters - FilterArray[ - Filter::ReferenceGathererFilter - ] - end - end - end -end diff --git a/lib/banzai/reference_extractor.rb b/lib/banzai/reference_extractor.rb index f4079538ec5..bf366962aef 100644 --- a/lib/banzai/reference_extractor.rb +++ b/lib/banzai/reference_extractor.rb @@ -1,28 +1,6 @@ module Banzai # Extract possible GFM references from an arbitrary String for further processing. class ReferenceExtractor - class << self - LAZY_KEY = :banzai_reference_extractor_lazy - - def lazy? - Thread.current[LAZY_KEY] - end - - def lazily(values = nil, &block) - return (values || block.call).uniq if lazy? - - begin - Thread.current[LAZY_KEY] = true - - values ||= block.call - - Banzai::LazyReference.load(values.uniq).uniq - ensure - Thread.current[LAZY_KEY] = false - end - end - end - def initialize @texts = [] end @@ -31,23 +9,21 @@ module Banzai @texts << Renderer.render(text, context) end - def references(type, context = {}) - filter = Banzai::Filter["#{type}_reference"] + def references(type, project, current_user = nil) + processor = Banzai::ReferenceParser[type]. + new(project, current_user) + + processor.process(html_documents) + end - context.merge!( - pipeline: :reference_extraction, + private - # ReferenceGathererFilter - reference_filter: filter - ) + def html_documents + # This ensures that we don't memoize anything until we have a number of + # text blobs to parse. + return [] if @texts.empty? - self.class.lazily do - @texts.flat_map do |html| - text_context = context.dup - result = Renderer.render_result(html, text_context) - result[:references][type] - end.uniq - end + @html_documents ||= @texts.map { |html| Nokogiri::HTML.fragment(html) } end end end diff --git a/lib/banzai/reference_parser.rb b/lib/banzai/reference_parser.rb new file mode 100644 index 00000000000..557bec4316e --- /dev/null +++ b/lib/banzai/reference_parser.rb @@ -0,0 +1,14 @@ +module Banzai + module ReferenceParser + # Returns the reference parser class for the given type + # + # Example: + # + # Banzai::ReferenceParser['issue'] + # + # This would return the `Banzai::ReferenceParser::IssueParser` class. + def self.[](name) + const_get("#{name.to_s.camelize}Parser") + end + end +end diff --git a/lib/banzai/reference_parser/base_parser.rb b/lib/banzai/reference_parser/base_parser.rb new file mode 100644 index 00000000000..3d7b9c4a024 --- /dev/null +++ b/lib/banzai/reference_parser/base_parser.rb @@ -0,0 +1,204 @@ +module Banzai + module ReferenceParser + # Base class for reference parsing classes. + # + # Each parser should also specify its reference type by calling + # `self.reference_type = ...` in the body of the class. The value of this + # method should be a symbol such as `:issue` or `:merge_request`. For + # example: + # + # class IssueParser < BaseParser + # self.reference_type = :issue + # end + # + # The reference type is used to determine what nodes to pass to the + # `referenced_by` method. + # + # Parser classes should either implement the instance method + # `references_relation` or overwrite `referenced_by`. The + # `references_relation` method is supposed to return an + # ActiveRecord::Relation used as a base relation for retrieving the objects + # referenced in a set of HTML nodes. + # + # Each class can implement two additional methods: + # + # * `nodes_user_can_reference`: returns an Array of nodes the given user can + # refer to. + # * `nodes_visible_to_user`: returns an Array of nodes that are visible to + # the given user. + # + # You only need to overwrite these methods if you want to tweak who can see + # which references. For example, the IssueParser class defines its own + # `nodes_visible_to_user` method so it can ensure users can only see issues + # they have access to. + class BaseParser + class << self + attr_accessor :reference_type + end + + # Returns the attribute name containing the value for every object to be + # parsed by the current parser. + # + # For example, for a parser class that returns "Animal" objects this + # attribute would be "data-animal". + def self.data_attribute + @data_attribute ||= "data-#{reference_type.to_s.dasherize}" + end + + def initialize(project = nil, current_user = nil) + @project = project + @current_user = current_user + end + + # Returns all the nodes containing references that the user can refer to. + def nodes_user_can_reference(user, nodes) + nodes + end + + # Returns all the nodes that are visible to the given user. + def nodes_visible_to_user(user, nodes) + projects = lazy { projects_for_nodes(nodes) } + project_attr = 'data-project' + + nodes.select do |node| + if node.has_attribute?(project_attr) + node_id = node.attr(project_attr).to_i + + if project && project.id == node_id + true + else + can?(user, :read_project, projects[node_id]) + end + else + true + end + end + end + + # Returns an Array of objects referenced by any of the given HTML nodes. + def referenced_by(nodes) + ids = unique_attribute_values(nodes, self.class.data_attribute) + + references_relation.where(id: ids) + end + + # Returns the ActiveRecord::Relation to use for querying references in the + # DB. + def references_relation + raise NotImplementedError, + "#{self.class} does not implement #{__method__}" + end + + # Returns a Hash containing attribute values per project ID. + # + # The returned Hash uses the following format: + # + # { project id => [value1, value2, ...] } + # + # nodes - An Array of HTML nodes to process. + # attribute - The name of the attribute (as a String) for which to gather + # values. + # + # Returns a Hash. + def gather_attributes_per_project(nodes, attribute) + per_project = Hash.new { |hash, key| hash[key] = Set.new } + + nodes.each do |node| + project_id = node.attr('data-project').to_i + id = node.attr(attribute) + + per_project[project_id] << id if id + end + + per_project + end + + # Returns a Hash containing objects for an attribute grouped per their + # IDs. + # + # The returned Hash uses the following format: + # + # { id value => row } + # + # nodes - An Array of HTML nodes to process. + # + # collection - The model or ActiveRecord relation to use for retrieving + # rows from the database. + # + # attribute - The name of the attribute containing the primary key values + # for every row. + # + # Returns a Hash. + def grouped_objects_for_nodes(nodes, collection, attribute) + return {} if nodes.empty? + + ids = unique_attribute_values(nodes, attribute) + + collection.where(id: ids).each_with_object({}) do |row, hash| + hash[row.id] = row + end + end + + # Returns an Array containing all unique values of an attribute of the + # given nodes. + def unique_attribute_values(nodes, attribute) + values = Set.new + + nodes.each do |node| + if node.has_attribute?(attribute) + values << node.attr(attribute) + end + end + + values.to_a + end + + # Processes the list of HTML documents and returns an Array containing all + # the references. + def process(documents) + type = self.class.reference_type + + nodes = documents.flat_map do |document| + Querying.css(document, "a[data-reference-type='#{type}'].gfm").to_a + end + + gather_references(nodes) + end + + # Gathers the references for the given HTML nodes. + def gather_references(nodes) + nodes = nodes_user_can_reference(current_user, nodes) + nodes = nodes_visible_to_user(current_user, nodes) + + referenced_by(nodes) + end + + # Returns a Hash containing the projects for a given list of HTML nodes. + # + # The returned Hash uses the following format: + # + # { project ID => project } + # + def projects_for_nodes(nodes) + @projects_for_nodes ||= + grouped_objects_for_nodes(nodes, Project, 'data-project') + end + + def can?(user, permission, subject) + Ability.abilities.allowed?(user, permission, subject) + end + + def find_projects_for_hash_keys(hash) + Project.where(id: hash.keys) + end + + private + + attr_reader :current_user, :project + + def lazy(&block) + Gitlab::Lazy.new(&block) + end + end + end +end diff --git a/lib/banzai/reference_parser/commit_parser.rb b/lib/banzai/reference_parser/commit_parser.rb new file mode 100644 index 00000000000..0fee9d267de --- /dev/null +++ b/lib/banzai/reference_parser/commit_parser.rb @@ -0,0 +1,34 @@ +module Banzai + module ReferenceParser + class CommitParser < BaseParser + self.reference_type = :commit + + def referenced_by(nodes) + commit_ids = commit_ids_per_project(nodes) + projects = find_projects_for_hash_keys(commit_ids) + + projects.flat_map do |project| + find_commits(project, commit_ids[project.id]) + end + end + + def commit_ids_per_project(nodes) + gather_attributes_per_project(nodes, self.class.data_attribute) + end + + def find_commits(project, ids) + commits = [] + + return commits unless project.valid_repo? + + ids.each do |id| + commit = project.commit(id) + + commits << commit if commit + end + + commits + end + end + end +end diff --git a/lib/banzai/reference_parser/commit_range_parser.rb b/lib/banzai/reference_parser/commit_range_parser.rb new file mode 100644 index 00000000000..69d01f8db15 --- /dev/null +++ b/lib/banzai/reference_parser/commit_range_parser.rb @@ -0,0 +1,38 @@ +module Banzai + module ReferenceParser + class CommitRangeParser < BaseParser + self.reference_type = :commit_range + + def referenced_by(nodes) + range_ids = commit_range_ids_per_project(nodes) + projects = find_projects_for_hash_keys(range_ids) + + projects.flat_map do |project| + find_ranges(project, range_ids[project.id]) + end + end + + def commit_range_ids_per_project(nodes) + gather_attributes_per_project(nodes, self.class.data_attribute) + end + + def find_ranges(project, range_ids) + ranges = [] + + range_ids.each do |id| + range = find_object(project, id) + + ranges << range if range + end + + ranges + end + + def find_object(project, id) + range = CommitRange.new(id, project) + + range.valid_commits? ? range : nil + end + end + end +end diff --git a/lib/banzai/reference_parser/external_issue_parser.rb b/lib/banzai/reference_parser/external_issue_parser.rb new file mode 100644 index 00000000000..a1264db2111 --- /dev/null +++ b/lib/banzai/reference_parser/external_issue_parser.rb @@ -0,0 +1,25 @@ +module Banzai + module ReferenceParser + class ExternalIssueParser < BaseParser + self.reference_type = :external_issue + + def referenced_by(nodes) + issue_ids = issue_ids_per_project(nodes) + projects = find_projects_for_hash_keys(issue_ids) + issues = [] + + projects.each do |project| + issue_ids[project.id].each do |id| + issues << ExternalIssue.new(id, project) + end + end + + issues + end + + def issue_ids_per_project(nodes) + gather_attributes_per_project(nodes, self.class.data_attribute) + end + end + end +end diff --git a/lib/banzai/reference_parser/issue_parser.rb b/lib/banzai/reference_parser/issue_parser.rb new file mode 100644 index 00000000000..24076e3d9ec --- /dev/null +++ b/lib/banzai/reference_parser/issue_parser.rb @@ -0,0 +1,40 @@ +module Banzai + module ReferenceParser + class IssueParser < BaseParser + self.reference_type = :issue + + def nodes_visible_to_user(user, nodes) + # It is not possible to check access rights for external issue trackers + return nodes if project && project.external_issue_tracker + + issues = issues_for_nodes(nodes) + + nodes.select do |node| + issue = issue_for_node(issues, node) + + issue ? can?(user, :read_issue, issue) : false + end + end + + def referenced_by(nodes) + issues = issues_for_nodes(nodes) + + nodes.map { |node| issue_for_node(issues, node) }.uniq + end + + def issues_for_nodes(nodes) + @issues_for_nodes ||= grouped_objects_for_nodes( + nodes, + Issue.all.includes(:author, :assignee, :project), + self.class.data_attribute + ) + end + + private + + def issue_for_node(issues, node) + issues[node.attr(self.class.data_attribute).to_i] + end + end + end +end diff --git a/lib/banzai/reference_parser/label_parser.rb b/lib/banzai/reference_parser/label_parser.rb new file mode 100644 index 00000000000..e5d1eb11d7f --- /dev/null +++ b/lib/banzai/reference_parser/label_parser.rb @@ -0,0 +1,11 @@ +module Banzai + module ReferenceParser + class LabelParser < BaseParser + self.reference_type = :label + + def references_relation + Label + end + end + end +end diff --git a/lib/banzai/reference_parser/merge_request_parser.rb b/lib/banzai/reference_parser/merge_request_parser.rb new file mode 100644 index 00000000000..c9a9ca79c09 --- /dev/null +++ b/lib/banzai/reference_parser/merge_request_parser.rb @@ -0,0 +1,11 @@ +module Banzai + module ReferenceParser + class MergeRequestParser < BaseParser + self.reference_type = :merge_request + + def references_relation + MergeRequest.includes(:author, :assignee, :target_project) + end + end + end +end diff --git a/lib/banzai/reference_parser/milestone_parser.rb b/lib/banzai/reference_parser/milestone_parser.rb new file mode 100644 index 00000000000..a000ac61e5c --- /dev/null +++ b/lib/banzai/reference_parser/milestone_parser.rb @@ -0,0 +1,11 @@ +module Banzai + module ReferenceParser + class MilestoneParser < BaseParser + self.reference_type = :milestone + + def references_relation + Milestone + end + end + end +end diff --git a/lib/banzai/reference_parser/snippet_parser.rb b/lib/banzai/reference_parser/snippet_parser.rb new file mode 100644 index 00000000000..fa71b3c952a --- /dev/null +++ b/lib/banzai/reference_parser/snippet_parser.rb @@ -0,0 +1,11 @@ +module Banzai + module ReferenceParser + class SnippetParser < BaseParser + self.reference_type = :snippet + + def references_relation + Snippet + end + end + end +end diff --git a/lib/banzai/reference_parser/user_parser.rb b/lib/banzai/reference_parser/user_parser.rb new file mode 100644 index 00000000000..a12b0d19560 --- /dev/null +++ b/lib/banzai/reference_parser/user_parser.rb @@ -0,0 +1,92 @@ +module Banzai + module ReferenceParser + class UserParser < BaseParser + self.reference_type = :user + + def referenced_by(nodes) + group_ids = [] + user_ids = [] + project_ids = [] + + nodes.each do |node| + if node.has_attribute?('data-group') + group_ids << node.attr('data-group').to_i + elsif node.has_attribute?(self.class.data_attribute) + user_ids << node.attr(self.class.data_attribute).to_i + elsif node.has_attribute?('data-project') + project_ids << node.attr('data-project').to_i + end + end + + find_users_for_groups(group_ids) | find_users(user_ids) | + find_users_for_projects(project_ids) + end + + def nodes_visible_to_user(user, nodes) + group_attr = 'data-group' + groups = lazy { grouped_objects_for_nodes(nodes, Group, group_attr) } + visible = [] + remaining = [] + + nodes.each do |node| + if node.has_attribute?(group_attr) + node_group = groups[node.attr(group_attr).to_i] + + if node_group && + can?(user, :read_group, node_group) + visible << node + end + # Remaining nodes will be processed by the parent class' + # implementation of this method. + else + remaining << node + end + end + + visible + super(current_user, remaining) + end + + def nodes_user_can_reference(current_user, nodes) + project_attr = 'data-project' + author_attr = 'data-author' + + projects = lazy { projects_for_nodes(nodes) } + users = lazy { grouped_objects_for_nodes(nodes, User, author_attr) } + + nodes.select do |node| + project_id = node.attr(project_attr) + user_id = node.attr(author_attr) + + if project && project_id && project.id == project_id.to_i + true + elsif project_id && user_id + project = projects[project_id.to_i] + user = users[user_id.to_i] + + project && user ? project.team.member?(user) : false + else + true + end + end + end + + def find_users(ids) + return [] if ids.empty? + + User.where(id: ids).to_a + end + + def find_users_for_groups(ids) + return [] if ids.empty? + + User.joins(:group_members).where(members: { source_id: ids }).to_a + end + + def find_users_for_projects(ids) + return [] if ids.empty? + + Project.where(id: ids).flat_map { |p| p.team.members.to_a } + end + end + end +end diff --git a/lib/gitlab/lazy.rb b/lib/gitlab/lazy.rb new file mode 100644 index 00000000000..2a659ae4c74 --- /dev/null +++ b/lib/gitlab/lazy.rb @@ -0,0 +1,34 @@ +module Gitlab + # A class that can be wrapped around an expensive method call so it's only + # executed when actually needed. + # + # Usage: + # + # object = Gitlab::Lazy.new { some_expensive_work_here } + # + # object['foo'] + # object.bar + class Lazy < BasicObject + def initialize(&block) + @block = block + end + + def method_missing(name, *args, &block) + __evaluate__ + + @result.__send__(name, *args, &block) + end + + def respond_to_missing?(name, include_private = false) + __evaluate__ + + @result.respond_to?(name, include_private) || super + end + + private + + def __evaluate__ + @result = @block.call unless defined?(@result) + end + end +end diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 13c4d64c99b..11c0b01f0dc 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -4,10 +4,9 @@ module Gitlab REFERABLES = %i(user issue label milestone merge_request snippet commit commit_range) attr_accessor :project, :current_user, :author - def initialize(project, current_user = nil, author = nil) + def initialize(project, current_user = nil) @project = project @current_user = current_user - @author = author @references = {} @@ -18,17 +17,21 @@ module Gitlab super(text, context.merge(project: project)) end + def references(type) + super(type, project, current_user) + end + REFERABLES.each do |type| define_method("#{type}s") do - @references[type] ||= references(type, reference_context) + @references[type] ||= references(type) end end def issues if project && project.jira_tracker? - @references[:external_issue] ||= references(:external_issue, reference_context) + @references[:external_issue] ||= references(:external_issue) else - @references[:issue] ||= references(:issue, reference_context) + @references[:issue] ||= references(:issue) end end @@ -46,11 +49,5 @@ module Gitlab @pattern = Regexp.union(patterns.compact) end - - private - - def reference_context - { project: project, current_user: current_user, author: author } - end end end diff --git a/spec/lib/banzai/filter/commit_range_reference_filter_spec.rb b/spec/lib/banzai/filter/commit_range_reference_filter_spec.rb index c2a8ad36c30..593bd6d5cac 100644 --- a/spec/lib/banzai/filter/commit_range_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/commit_range_reference_filter_spec.rb @@ -98,11 +98,6 @@ describe Banzai::Filter::CommitRangeReferenceFilter, lib: true do expect(link).not_to match %r(https?://) expect(link).to eq urls.namespace_project_compare_url(project.namespace, project, from: commit1.id, to: commit2.id, only_path: true) end - - it 'adds to the results hash' do - result = reference_pipeline_result("See #{reference}") - expect(result[:references][:commit_range]).not_to be_empty - end end context 'cross-project reference' do @@ -135,11 +130,6 @@ describe Banzai::Filter::CommitRangeReferenceFilter, lib: true do exp = act = "Fixed #{project2.to_reference}@#{commit1.id}...#{commit2.id.reverse}" expect(reference_filter(act).to_html).to eq exp end - - it 'adds to the results hash' do - result = reference_pipeline_result("See #{reference}") - expect(result[:references][:commit_range]).not_to be_empty - end end context 'cross-project URL reference' do @@ -173,10 +163,5 @@ describe Banzai::Filter::CommitRangeReferenceFilter, lib: true do exp = act = "Fixed #{project2.to_reference}@#{commit1.id}...#{commit2.id.reverse}" expect(reference_filter(act).to_html).to eq exp end - - it 'adds to the results hash' do - result = reference_pipeline_result("See #{reference}") - expect(result[:references][:commit_range]).not_to be_empty - end end end diff --git a/spec/lib/banzai/filter/commit_reference_filter_spec.rb b/spec/lib/banzai/filter/commit_reference_filter_spec.rb index 63a32d9d455..d46d3f1489e 100644 --- a/spec/lib/banzai/filter/commit_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/commit_reference_filter_spec.rb @@ -93,11 +93,6 @@ describe Banzai::Filter::CommitReferenceFilter, lib: true do expect(link).not_to match %r(https?://) expect(link).to eq urls.namespace_project_commit_url(project.namespace, project, reference, only_path: true) end - - it 'adds to the results hash' do - result = reference_pipeline_result("See #{reference}") - expect(result[:references][:commit]).not_to be_empty - end end context 'cross-project reference' do @@ -124,11 +119,6 @@ describe Banzai::Filter::CommitReferenceFilter, lib: true do exp = act = "Committed #{invalidate_reference(reference)}" expect(reference_filter(act).to_html).to eq exp end - - it 'adds to the results hash' do - result = reference_pipeline_result("See #{reference}") - expect(result[:references][:commit]).not_to be_empty - end end context 'cross-project URL reference' do @@ -154,10 +144,5 @@ describe Banzai::Filter::CommitReferenceFilter, lib: true do act = "Committed #{invalidate_reference(reference)}" expect(reference_filter(act).to_html).to match(/<a.+>#{Regexp.escape(invalidate_reference(reference))}<\/a>/) end - - it 'adds to the results hash' do - result = reference_pipeline_result("See #{reference}") - expect(result[:references][:commit]).not_to be_empty - end end end diff --git a/spec/lib/banzai/filter/issue_reference_filter_spec.rb b/spec/lib/banzai/filter/issue_reference_filter_spec.rb index 266ebef33d6..8e6a264970d 100644 --- a/spec/lib/banzai/filter/issue_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/issue_reference_filter_spec.rb @@ -91,11 +91,6 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do expect(link).to eq helper.url_for_issue(issue.iid, project, only_path: true) end - it 'adds to the results hash' do - result = reference_pipeline_result("Fixed #{reference}") - expect(result[:references][:issue]).to eq [issue] - end - it 'does not process links containing issue numbers followed by text' do href = "#{reference}st" doc = reference_filter("<a href='#{href}'></a>") @@ -136,11 +131,6 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do expect(reference_filter(act).to_html).to eq exp end - - it 'adds to the results hash' do - result = reference_pipeline_result("Fixed #{reference}") - expect(result[:references][:issue]).to eq [issue] - end end context 'cross-project URL reference' do @@ -160,11 +150,6 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do doc = reference_filter("Fixed (#{reference}.)") expect(doc.to_html).to match(/\(<a.+>#{Regexp.escape(issue.to_reference(project))} \(comment 123\)<\/a>\.\)/) end - - it 'adds to the results hash' do - result = reference_pipeline_result("Fixed #{reference}") - expect(result[:references][:issue]).to eq [issue] - end end context 'cross-project reference in link href' do @@ -184,11 +169,6 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do doc = reference_filter("Fixed (#{reference}.)") expect(doc.to_html).to match(/\(<a.+>Reference<\/a>\.\)/) end - - it 'adds to the results hash' do - result = reference_pipeline_result("Fixed #{reference}") - expect(result[:references][:issue]).to eq [issue] - end end context 'cross-project URL in link href' do @@ -208,10 +188,5 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do doc = reference_filter("Fixed (#{reference}.)") expect(doc.to_html).to match(/\(<a.+>Reference<\/a>\.\)/) end - - it 'adds to the results hash' do - result = reference_pipeline_result("Fixed #{reference}") - expect(result[:references][:issue]).to eq [issue] - end end end diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index b0a38e7c251..f1064a701d8 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -48,11 +48,6 @@ describe Banzai::Filter::LabelReferenceFilter, lib: true do expect(link).to eq urls.namespace_project_issues_path(project.namespace, project, label_name: label.name) end - it 'adds to the results hash' do - result = reference_pipeline_result("Label #{reference}") - expect(result[:references][:label]).to eq [label] - end - describe 'label span element' do it 'includes default classes' do doc = reference_filter("Label #{reference}") @@ -170,11 +165,6 @@ describe Banzai::Filter::LabelReferenceFilter, lib: true do expect(link).to have_attribute('data-label') expect(link.attr('data-label')).to eq label.id.to_s end - - it 'adds to the results hash' do - result = reference_pipeline_result("Label #{reference}") - expect(result[:references][:label]).to eq [label] - end end describe 'cross project label references' do diff --git a/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb b/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb index 352710df307..3185e41fe5c 100644 --- a/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb @@ -78,11 +78,6 @@ describe Banzai::Filter::MergeRequestReferenceFilter, lib: true do expect(link).not_to match %r(https?://) expect(link).to eq urls.namespace_project_merge_request_url(project.namespace, project, merge, only_path: true) end - - it 'adds to the results hash' do - result = reference_pipeline_result("Merge #{reference}") - expect(result[:references][:merge_request]).to eq [merge] - end end context 'cross-project reference' do @@ -109,11 +104,6 @@ describe Banzai::Filter::MergeRequestReferenceFilter, lib: true do expect(reference_filter(act).to_html).to eq exp end - - it 'adds to the results hash' do - result = reference_pipeline_result("Merge #{reference}") - expect(result[:references][:merge_request]).to eq [merge] - end end context 'cross-project URL reference' do @@ -133,10 +123,5 @@ describe Banzai::Filter::MergeRequestReferenceFilter, lib: true do doc = reference_filter("Merge (#{reference}.)") expect(doc.to_html).to match(/\(<a.+>#{Regexp.escape(merge.to_reference(project))} \(diffs, comment 123\)<\/a>\.\)/) end - - it 'adds to the results hash' do - result = reference_pipeline_result("Merge #{reference}") - expect(result[:references][:merge_request]).to eq [merge] - end end end diff --git a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb index bdf48eabb0e..9424f2363e1 100644 --- a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb @@ -48,11 +48,6 @@ describe Banzai::Filter::MilestoneReferenceFilter, lib: true do namespace_project_milestone_path(project.namespace, project, milestone) end - it 'adds to the results hash' do - result = reference_pipeline_result("Milestone #{reference}") - expect(result[:references][:milestone]).to eq [milestone] - end - context 'Integer-based references' do it 'links to a valid reference' do doc = reference_filter("See #{reference}") @@ -151,11 +146,6 @@ describe Banzai::Filter::MilestoneReferenceFilter, lib: true do expect(link).to have_attribute('data-milestone') expect(link.attr('data-milestone')).to eq milestone.id.to_s end - - it 'adds to the results hash' do - result = reference_pipeline_result("Milestone #{reference}") - expect(result[:references][:milestone]).to eq [milestone] - end end describe 'cross project milestone references' do diff --git a/spec/lib/banzai/filter/redactor_filter_spec.rb b/spec/lib/banzai/filter/redactor_filter_spec.rb index c2c2fd0eb6a..697d10bbf70 100644 --- a/spec/lib/banzai/filter/redactor_filter_spec.rb +++ b/spec/lib/banzai/filter/redactor_filter_spec.rb @@ -16,11 +16,23 @@ describe Banzai::Filter::RedactorFilter, lib: true do end context 'with data-project' do + let(:parser_class) do + Class.new(Banzai::ReferenceParser::BaseParser) do + self.reference_type = :test + end + end + + before do + allow(Banzai::ReferenceParser).to receive(:[]). + with('test'). + and_return(parser_class) + end + it 'removes unpermitted Project references' do user = create(:user) project = create(:empty_project) - link = reference_link(project: project.id, reference_filter: 'ReferenceFilter') + link = reference_link(project: project.id, reference_type: 'test') doc = filter(link, current_user: user) expect(doc.css('a').length).to eq 0 @@ -31,14 +43,14 @@ describe Banzai::Filter::RedactorFilter, lib: true do project = create(:empty_project) project.team << [user, :master] - link = reference_link(project: project.id, reference_filter: 'ReferenceFilter') + link = reference_link(project: project.id, reference_type: 'test') doc = filter(link, current_user: user) expect(doc.css('a').length).to eq 1 end it 'handles invalid Project references' do - link = reference_link(project: 12345, reference_filter: 'ReferenceFilter') + link = reference_link(project: 12345, reference_type: 'test') expect { filter(link) }.not_to raise_error end @@ -51,7 +63,7 @@ describe Banzai::Filter::RedactorFilter, lib: true do project = create(:empty_project, :public) issue = create(:issue, :confidential, project: project) - link = reference_link(project: project.id, issue: issue.id, reference_filter: 'IssueReferenceFilter') + link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue') doc = filter(link, current_user: non_member) expect(doc.css('a').length).to eq 0 @@ -62,7 +74,7 @@ describe Banzai::Filter::RedactorFilter, lib: true do project = create(:empty_project, :public) issue = create(:issue, :confidential, project: project, author: author) - link = reference_link(project: project.id, issue: issue.id, reference_filter: 'IssueReferenceFilter') + link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue') doc = filter(link, current_user: author) expect(doc.css('a').length).to eq 1 @@ -73,7 +85,7 @@ describe Banzai::Filter::RedactorFilter, lib: true do project = create(:empty_project, :public) issue = create(:issue, :confidential, project: project, assignee: assignee) - link = reference_link(project: project.id, issue: issue.id, reference_filter: 'IssueReferenceFilter') + link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue') doc = filter(link, current_user: assignee) expect(doc.css('a').length).to eq 1 @@ -85,7 +97,7 @@ describe Banzai::Filter::RedactorFilter, lib: true do project.team << [member, :developer] issue = create(:issue, :confidential, project: project) - link = reference_link(project: project.id, issue: issue.id, reference_filter: 'IssueReferenceFilter') + link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue') doc = filter(link, current_user: member) expect(doc.css('a').length).to eq 1 @@ -96,7 +108,7 @@ describe Banzai::Filter::RedactorFilter, lib: true do project = create(:empty_project, :public) issue = create(:issue, :confidential, project: project) - link = reference_link(project: project.id, issue: issue.id, reference_filter: 'IssueReferenceFilter') + link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue') doc = filter(link, current_user: admin) expect(doc.css('a').length).to eq 1 @@ -108,7 +120,7 @@ describe Banzai::Filter::RedactorFilter, lib: true do project = create(:empty_project, :public) issue = create(:issue, project: project) - link = reference_link(project: project.id, issue: issue.id, reference_filter: 'IssueReferenceFilter') + link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue') doc = filter(link, current_user: user) expect(doc.css('a').length).to eq 1 @@ -121,7 +133,7 @@ describe Banzai::Filter::RedactorFilter, lib: true do user = create(:user) group = create(:group, :private) - link = reference_link(group: group.id, reference_filter: 'UserReferenceFilter') + link = reference_link(group: group.id, reference_type: 'user') doc = filter(link, current_user: user) expect(doc.css('a').length).to eq 0 @@ -132,14 +144,14 @@ describe Banzai::Filter::RedactorFilter, lib: true do group = create(:group, :private) group.add_developer(user) - link = reference_link(group: group.id, reference_filter: 'UserReferenceFilter') + link = reference_link(group: group.id, reference_type: 'user') 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: 12345, reference_filter: 'UserReferenceFilter') + link = reference_link(group: 12345, reference_type: 'user') expect { filter(link) }.not_to raise_error end @@ -149,7 +161,7 @@ describe Banzai::Filter::RedactorFilter, lib: true do it 'allows any User reference' do user = create(:user) - link = reference_link(user: user.id, reference_filter: 'UserReferenceFilter') + link = reference_link(user: user.id, reference_type: 'user') doc = filter(link) expect(doc.css('a').length).to eq 1 diff --git a/spec/lib/banzai/filter/reference_gatherer_filter_spec.rb b/spec/lib/banzai/filter/reference_gatherer_filter_spec.rb deleted file mode 100644 index c8b1dfdf944..00000000000 --- a/spec/lib/banzai/filter/reference_gatherer_filter_spec.rb +++ /dev/null @@ -1,87 +0,0 @@ -require 'spec_helper' - -describe Banzai::Filter::ReferenceGathererFilter, lib: true 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: 'IssueReferenceFilter') - 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: 'IssueReferenceFilter') - 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: 'IssueReferenceFilter') - - 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: 'UserReferenceFilter') - 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: 'UserReferenceFilter') - 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: 'UserReferenceFilter') - - 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: 'UserReferenceFilter') - result = pipeline_result(link) - - expect(result[:references][:user]).to eq([user]) - end - end - end -end diff --git a/spec/lib/banzai/filter/snippet_reference_filter_spec.rb b/spec/lib/banzai/filter/snippet_reference_filter_spec.rb index 26466fbb180..5068ddd7faa 100644 --- a/spec/lib/banzai/filter/snippet_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/snippet_reference_filter_spec.rb @@ -77,11 +77,6 @@ describe Banzai::Filter::SnippetReferenceFilter, lib: true do expect(link).not_to match %r(https?://) expect(link).to eq urls.namespace_project_snippet_url(project.namespace, project, snippet, only_path: true) end - - it 'adds to the results hash' do - result = reference_pipeline_result("Snippet #{reference}") - expect(result[:references][:snippet]).to eq [snippet] - end end context 'cross-project reference' do @@ -107,11 +102,6 @@ describe Banzai::Filter::SnippetReferenceFilter, lib: true do expect(reference_filter(act).to_html).to eq exp end - - it 'adds to the results hash' do - result = reference_pipeline_result("Snippet #{reference}") - expect(result[:references][:snippet]).to eq [snippet] - end end context 'cross-project URL reference' do @@ -137,10 +127,5 @@ describe Banzai::Filter::SnippetReferenceFilter, lib: true do expect(reference_filter(act).to_html).to match(/<a.+>#{Regexp.escape(invalidate_reference(reference))}<\/a>/) end - - it 'adds to the results hash' do - result = reference_pipeline_result("Snippet #{reference}") - expect(result[:references][:snippet]).to eq [snippet] - end end end diff --git a/spec/lib/banzai/filter/user_reference_filter_spec.rb b/spec/lib/banzai/filter/user_reference_filter_spec.rb index 8bdebae1841..d7dfd6699ef 100644 --- a/spec/lib/banzai/filter/user_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/user_reference_filter_spec.rb @@ -31,28 +31,22 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do end it 'supports a special @all mention' do - doc = reference_filter("Hey #{reference}") + doc = reference_filter("Hey #{reference}", author: user) expect(doc.css('a').length).to eq 1 expect(doc.css('a').first.attr('href')) .to eq urls.namespace_project_url(project.namespace, project) end - context "when the author is a member of the project" do + it 'includes a data-author attribute when there is an author' do + doc = reference_filter(reference, author: user) - it 'adds to the results hash' do - result = reference_pipeline_result("Hey #{reference}", author: project.creator) - expect(result[:references][:user]).to eq [project.creator] - end + expect(doc.css('a').first.attr('data-author')).to eq(user.id.to_s) end - context "when the author is not a member of the project" do - - let(:other_user) { create(:user) } + it 'does not include a data-author attribute when there is no author' do + doc = reference_filter(reference) - it "doesn't add to the results hash" do - result = reference_pipeline_result("Hey #{reference}", author: other_user) - expect(result[:references][:user]).to eq [] - end + expect(doc.css('a').first.has_attribute?('data-author')).to eq(false) end end @@ -83,11 +77,6 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do 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 = reference_pipeline_result("Hey #{reference}") - expect(result[:references][:user]).to eq [user] - end end context 'mentioning a group' do @@ -106,11 +95,6 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do 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 = reference_pipeline_result("Hey #{reference}") - expect(result[:references][:user]).to eq group.users - end end it 'links with adjacent text' do @@ -151,10 +135,5 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do 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 = reference_pipeline_result("Hey #{reference}") - expect(result[:references][:user]).to eq [user] - end end end diff --git a/spec/lib/banzai/reference_parser/base_parser_spec.rb b/spec/lib/banzai/reference_parser/base_parser_spec.rb new file mode 100644 index 00000000000..543b4786d84 --- /dev/null +++ b/spec/lib/banzai/reference_parser/base_parser_spec.rb @@ -0,0 +1,237 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::BaseParser, lib: true do + include ReferenceParserHelpers + + let(:user) { create(:user) } + let(:project) { create(:empty_project, :public) } + + subject do + klass = Class.new(described_class) do + self.reference_type = :foo + end + + klass.new(project, user) + end + + describe '.reference_type=' do + it 'sets the reference type' do + dummy = Class.new(described_class) + dummy.reference_type = :foo + + expect(dummy.reference_type).to eq(:foo) + end + end + + describe '#nodes_visible_to_user' do + let(:link) { empty_html_link } + + context 'when the link has a data-project attribute' do + it 'returns the nodes if the attribute value equals the current project ID' do + link['data-project'] = project.id.to_s + + expect(Ability.abilities).not_to receive(:allowed?) + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + + it 'returns the nodes if the user can read the project' do + other_project = create(:empty_project, :public) + + link['data-project'] = other_project.id.to_s + + expect(Ability.abilities).to receive(:allowed?). + with(user, :read_project, other_project). + and_return(true) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + + it 'returns an empty Array when the attribute value is empty' do + link['data-project'] = '' + + expect(subject.nodes_visible_to_user(user, [link])).to eq([]) + end + + it 'returns an empty Array when the user can not read the project' do + other_project = create(:empty_project, :public) + + link['data-project'] = other_project.id.to_s + + expect(Ability.abilities).to receive(:allowed?). + with(user, :read_project, other_project). + and_return(false) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([]) + end + end + + context 'when the link does not have a data-project attribute' do + it 'returns the nodes' do + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + end + end + + describe '#nodes_user_can_reference' do + it 'returns the nodes' do + link = double(:link) + + expect(subject.nodes_user_can_reference(user, [link])).to eq([link]) + end + end + + describe '#referenced_by' do + context 'when references_relation is implemented' do + it 'returns a collection of objects' do + links = Nokogiri::HTML.fragment("<a data-foo='#{user.id}'></a>"). + children + + expect(subject).to receive(:references_relation).and_return(User) + expect(subject.referenced_by(links)).to eq([user]) + end + end + + context 'when references_relation is not implemented' do + it 'raises NotImplementedError' do + links = Nokogiri::HTML.fragment('<a data-foo="1"></a>').children + + expect { subject.referenced_by(links) }. + to raise_error(NotImplementedError) + end + end + end + + describe '#references_relation' do + it 'raises NotImplementedError' do + expect { subject.references_relation }.to raise_error(NotImplementedError) + end + end + + describe '#gather_attributes_per_project' do + it 'returns a Hash containing attribute values per project' do + link = Nokogiri::HTML.fragment('<a data-project="1" data-foo="2"></a>'). + children[0] + + hash = subject.gather_attributes_per_project([link], 'data-foo') + + expect(hash).to be_an_instance_of(Hash) + + expect(hash[1].to_a).to eq(['2']) + end + end + + describe '#grouped_objects_for_nodes' do + it 'returns a Hash grouping objects per ID' do + nodes = [double(:node)] + + expect(subject).to receive(:unique_attribute_values). + with(nodes, 'data-user'). + and_return([user.id]) + + hash = subject.grouped_objects_for_nodes(nodes, User, 'data-user') + + expect(hash).to eq({ user.id => user }) + end + + it 'returns an empty Hash when the list of nodes is empty' do + expect(subject.grouped_objects_for_nodes([], User, 'data-user')).to eq({}) + end + end + + describe '#unique_attribute_values' do + it 'returns an Array of unique values' do + link = double(:link) + + expect(link).to receive(:has_attribute?). + with('data-foo'). + twice. + and_return(true) + + expect(link).to receive(:attr). + with('data-foo'). + twice. + and_return('1') + + nodes = [link, link] + + expect(subject.unique_attribute_values(nodes, 'data-foo')).to eq(['1']) + end + end + + describe '#process' do + it 'gathers the references for every node matching the reference type' do + dummy = Class.new(described_class) do + self.reference_type = :test + end + + instance = dummy.new(project, user) + document = Nokogiri::HTML.fragment('<a class="gfm"></a><a class="gfm" data-reference-type="test"></a>') + + expect(instance).to receive(:gather_references). + with([document.children[1]]). + and_return([user]) + + expect(instance.process([document])).to eq([user]) + end + end + + describe '#gather_references' do + let(:link) { double(:link) } + + it 'does not process links a user can not reference' do + expect(subject).to receive(:nodes_user_can_reference). + with(user, [link]). + and_return([]) + + expect(subject).to receive(:referenced_by).with([]) + + subject.gather_references([link]) + end + + it 'does not process links a user can not see' do + expect(subject).to receive(:nodes_user_can_reference). + with(user, [link]). + and_return([link]) + + expect(subject).to receive(:nodes_visible_to_user). + with(user, [link]). + and_return([]) + + expect(subject).to receive(:referenced_by).with([]) + + subject.gather_references([link]) + end + + it 'returns the references if a user can reference and see a link' do + expect(subject).to receive(:nodes_user_can_reference). + with(user, [link]). + and_return([link]) + + expect(subject).to receive(:nodes_visible_to_user). + with(user, [link]). + and_return([link]) + + expect(subject).to receive(:referenced_by).with([link]) + + subject.gather_references([link]) + end + end + + describe '#can?' do + it 'delegates the permissions check to the Ability class' do + user = double(:user) + + expect(Ability.abilities).to receive(:allowed?). + with(user, :read_project, project) + + subject.can?(user, :read_project, project) + end + end + + describe '#find_projects_for_hash_keys' do + it 'returns a list of Projects' do + expect(subject.find_projects_for_hash_keys(project.id => project)). + to eq([project]) + end + end +end diff --git a/spec/lib/banzai/reference_parser/commit_parser_spec.rb b/spec/lib/banzai/reference_parser/commit_parser_spec.rb new file mode 100644 index 00000000000..0b76d29fce0 --- /dev/null +++ b/spec/lib/banzai/reference_parser/commit_parser_spec.rb @@ -0,0 +1,113 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::CommitParser, lib: true do + include ReferenceParserHelpers + + let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } + subject { described_class.new(project, user) } + let(:link) { empty_html_link } + + describe '#referenced_by' do + context 'when the link has a data-project attribute' do + before do + link['data-project'] = project.id.to_s + end + + context 'when the link has a data-commit attribute' do + before do + link['data-commit'] = '123' + end + + it 'returns an Array of commits' do + commit = double(:commit) + + allow_any_instance_of(Project).to receive(:valid_repo?). + and_return(true) + + expect(subject).to receive(:find_commits). + with(project, ['123']). + and_return([commit]) + + expect(subject.referenced_by([link])).to eq([commit]) + end + + it 'returns an empty Array when the commit could not be found' do + allow_any_instance_of(Project).to receive(:valid_repo?). + and_return(true) + + expect(subject).to receive(:find_commits). + with(project, ['123']). + and_return([]) + + expect(subject.referenced_by([link])).to eq([]) + end + + it 'skips projects without valid repositories' do + allow_any_instance_of(Project).to receive(:valid_repo?). + and_return(false) + + expect(subject.referenced_by([link])).to eq([]) + end + end + + context 'when the link does not have a data-commit attribute' do + it 'returns an empty Array' do + allow_any_instance_of(Project).to receive(:valid_repo?). + and_return(true) + + expect(subject.referenced_by([link])).to eq([]) + end + end + end + + context 'when the link does not have a data-project attribute' do + it 'returns an empty Array' do + allow_any_instance_of(Project).to receive(:valid_repo?). + and_return(true) + + expect(subject.referenced_by([link])).to eq([]) + end + end + end + + describe '#commit_ids_per_project' do + before do + link['data-project'] = project.id.to_s + end + + it 'returns a Hash containing commit IDs per project' do + link['data-commit'] = '123' + + hash = subject.commit_ids_per_project([link]) + + expect(hash).to be_an_instance_of(Hash) + + expect(hash[project.id].to_a).to eq(['123']) + end + + it 'does not add a project when the data-commit attribute is empty' do + hash = subject.commit_ids_per_project([link]) + + expect(hash).to be_empty + end + end + + describe '#find_commits' do + it 'returns an Array of commit objects' do + commit = double(:commit) + + expect(project).to receive(:commit).with('123').and_return(commit) + expect(project).to receive(:valid_repo?).and_return(true) + + expect(subject.find_commits(project, %w{123})).to eq([commit]) + end + + it 'skips commit IDs for which no commit could be found' do + expect(project).to receive(:commit).with('123').and_return(nil) + expect(project).to receive(:valid_repo?).and_return(true) + + expect(subject.find_commits(project, %w{123})).to eq([]) + end + end +end diff --git a/spec/lib/banzai/reference_parser/commit_range_parser_spec.rb b/spec/lib/banzai/reference_parser/commit_range_parser_spec.rb new file mode 100644 index 00000000000..ba982f38542 --- /dev/null +++ b/spec/lib/banzai/reference_parser/commit_range_parser_spec.rb @@ -0,0 +1,120 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::CommitRangeParser, lib: true do + include ReferenceParserHelpers + + let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } + subject { described_class.new(project, user) } + let(:link) { empty_html_link } + + describe '#referenced_by' do + context 'when the link has a data-project attribute' do + before do + link['data-project'] = project.id.to_s + end + + context 'when the link as a data-commit-range attribute' do + before do + link['data-commit-range'] = '123..456' + end + + it 'returns an Array of commit ranges' do + range = double(:range) + + expect(subject).to receive(:find_object). + with(project, '123..456'). + and_return(range) + + expect(subject.referenced_by([link])).to eq([range]) + end + + it 'returns an empty Array when the commit range could not be found' do + expect(subject).to receive(:find_object). + with(project, '123..456'). + and_return(nil) + + expect(subject.referenced_by([link])).to eq([]) + end + end + + context 'when the link does not have a data-commit-range attribute' do + it 'returns an empty Array' do + expect(subject.referenced_by([link])).to eq([]) + end + end + end + + context 'when the link does not have a data-project attribute' do + it 'returns an empty Array' do + expect(subject.referenced_by([link])).to eq([]) + end + end + end + + describe '#commit_range_ids_per_project' do + before do + link['data-project'] = project.id.to_s + end + + it 'returns a Hash containing range IDs per project' do + link['data-commit-range'] = '123..456' + + hash = subject.commit_range_ids_per_project([link]) + + expect(hash).to be_an_instance_of(Hash) + + expect(hash[project.id].to_a).to eq(['123..456']) + end + + it 'does not add a project when the data-commit-range attribute is empty' do + hash = subject.commit_range_ids_per_project([link]) + + expect(hash).to be_empty + end + end + + describe '#find_ranges' do + it 'returns an Array of range objects' do + range = double(:commit) + + expect(subject).to receive(:find_object). + with(project, '123..456'). + and_return(range) + + expect(subject.find_ranges(project, ['123..456'])).to eq([range]) + end + + it 'skips ranges that could not be found' do + expect(subject).to receive(:find_object). + with(project, '123..456'). + and_return(nil) + + expect(subject.find_ranges(project, ['123..456'])).to eq([]) + end + end + + describe '#find_object' do + let(:range) { double(:range) } + + before do + expect(CommitRange).to receive(:new).and_return(range) + end + + context 'when the range has valid commits' do + it 'returns the commit range' do + expect(range).to receive(:valid_commits?).and_return(true) + + expect(subject.find_object(project, '123..456')).to eq(range) + end + end + + context 'when the range does not have any valid commits' do + it 'returns nil' do + expect(range).to receive(:valid_commits?).and_return(false) + + expect(subject.find_object(project, '123..456')).to be_nil + end + end + end +end diff --git a/spec/lib/banzai/reference_parser/external_issue_parser_spec.rb b/spec/lib/banzai/reference_parser/external_issue_parser_spec.rb new file mode 100644 index 00000000000..a6ef8394fe7 --- /dev/null +++ b/spec/lib/banzai/reference_parser/external_issue_parser_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::ExternalIssueParser, lib: true do + include ReferenceParserHelpers + + let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } + subject { described_class.new(project, user) } + let(:link) { empty_html_link } + + describe '#referenced_by' do + context 'when the link has a data-project attribute' do + before do + link['data-project'] = project.id.to_s + end + + context 'when the link has a data-external-issue attribute' do + it 'returns an Array of ExternalIssue instances' do + link['data-external-issue'] = '123' + + refs = subject.referenced_by([link]) + + expect(refs).to eq([ExternalIssue.new('123', project)]) + end + end + + context 'when the link does not have a data-external-issue attribute' do + it 'returns an empty Array' do + expect(subject.referenced_by([link])).to eq([]) + end + end + end + + context 'when the link does not have a data-project attribute' do + it 'returns an empty Array' do + expect(subject.referenced_by([link])).to eq([]) + end + end + end + + describe '#issue_ids_per_project' do + before do + link['data-project'] = project.id.to_s + end + + it 'returns a Hash containing range IDs per project' do + link['data-external-issue'] = '123' + + hash = subject.issue_ids_per_project([link]) + + expect(hash).to be_an_instance_of(Hash) + + expect(hash[project.id].to_a).to eq(['123']) + end + + it 'does not add a project when the data-external-issue attribute is empty' do + hash = subject.issue_ids_per_project([link]) + + expect(hash).to be_empty + end + end +end diff --git a/spec/lib/banzai/reference_parser/issue_parser_spec.rb b/spec/lib/banzai/reference_parser/issue_parser_spec.rb new file mode 100644 index 00000000000..514c752546d --- /dev/null +++ b/spec/lib/banzai/reference_parser/issue_parser_spec.rb @@ -0,0 +1,79 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::IssueParser, lib: true do + include ReferenceParserHelpers + + let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } + let(:issue) { create(:issue, project: project) } + subject { described_class.new(project, user) } + let(:link) { empty_html_link } + + describe '#nodes_visible_to_user' do + context 'when the link has a data-issue attribute' do + before do + link['data-issue'] = issue.id.to_s + end + + it 'returns the nodes when the user can read the issue' do + expect(Ability.abilities).to receive(:allowed?). + with(user, :read_issue, issue). + and_return(true) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + + it 'returns an empty Array when the user can not read the issue' do + expect(Ability.abilities).to receive(:allowed?). + with(user, :read_issue, issue). + and_return(false) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([]) + end + end + + context 'when the link does not have a data-issue attribute' do + it 'returns an empty Array' do + expect(subject.nodes_visible_to_user(user, [link])).to eq([]) + end + end + + context 'when the project uses an external issue tracker' do + it 'returns all nodes' do + link = double(:link) + + expect(project).to receive(:external_issue_tracker).and_return(true) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + end + end + + describe '#referenced_by' do + context 'when the link has a data-issue attribute' do + context 'using an existing issue ID' do + before do + link['data-issue'] = issue.id.to_s + end + + it 'returns an Array of issues' do + expect(subject.referenced_by([link])).to eq([issue]) + end + + it 'returns an empty Array when the list of nodes is empty' do + expect(subject.referenced_by([link])).to eq([issue]) + expect(subject.referenced_by([])).to eq([]) + end + end + end + end + + describe '#issues_for_nodes' do + it 'returns a Hash containing the issues for a list of nodes' do + link['data-issue'] = issue.id.to_s + nodes = [link] + + expect(subject.issues_for_nodes(nodes)).to eq({ issue.id => issue }) + end + end +end diff --git a/spec/lib/banzai/reference_parser/label_parser_spec.rb b/spec/lib/banzai/reference_parser/label_parser_spec.rb new file mode 100644 index 00000000000..77fda47f0e7 --- /dev/null +++ b/spec/lib/banzai/reference_parser/label_parser_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::LabelParser, lib: true do + include ReferenceParserHelpers + + let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } + let(:label) { create(:label, project: project) } + subject { described_class.new(project, user) } + let(:link) { empty_html_link } + + describe '#referenced_by' do + describe 'when the link has a data-label attribute' do + context 'using an existing label ID' do + it 'returns an Array of labels' do + link['data-label'] = label.id.to_s + + expect(subject.referenced_by([link])).to eq([label]) + end + end + + context 'using a non-existing label ID' do + it 'returns an empty Array' do + link['data-label'] = '' + + expect(subject.referenced_by([link])).to eq([]) + end + end + end + end +end diff --git a/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb b/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb new file mode 100644 index 00000000000..cf89ad598ea --- /dev/null +++ b/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb @@ -0,0 +1,30 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::MergeRequestParser, lib: true do + include ReferenceParserHelpers + + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request) } + subject { described_class.new(merge_request.target_project, user) } + let(:link) { empty_html_link } + + describe '#referenced_by' do + describe 'when the link has a data-merge-request attribute' do + context 'using an existing merge request ID' do + it 'returns an Array of merge requests' do + link['data-merge-request'] = merge_request.id.to_s + + expect(subject.referenced_by([link])).to eq([merge_request]) + end + end + + context 'using a non-existing merge request ID' do + it 'returns an empty Array' do + link['data-merge-request'] = '' + + expect(subject.referenced_by([link])).to eq([]) + end + end + end + end +end diff --git a/spec/lib/banzai/reference_parser/milestone_parser_spec.rb b/spec/lib/banzai/reference_parser/milestone_parser_spec.rb new file mode 100644 index 00000000000..6aa45a22cc4 --- /dev/null +++ b/spec/lib/banzai/reference_parser/milestone_parser_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::MilestoneParser, lib: true do + include ReferenceParserHelpers + + let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } + let(:milestone) { create(:milestone, project: project) } + subject { described_class.new(project, user) } + let(:link) { empty_html_link } + + describe '#referenced_by' do + describe 'when the link has a data-milestone attribute' do + context 'using an existing milestone ID' do + it 'returns an Array of milestones' do + link['data-milestone'] = milestone.id.to_s + + expect(subject.referenced_by([link])).to eq([milestone]) + end + end + + context 'using a non-existing milestone ID' do + it 'returns an empty Array' do + link['data-milestone'] = '' + + expect(subject.referenced_by([link])).to eq([]) + end + end + end + end +end diff --git a/spec/lib/banzai/reference_parser/snippet_parser_spec.rb b/spec/lib/banzai/reference_parser/snippet_parser_spec.rb new file mode 100644 index 00000000000..59127b7c5d1 --- /dev/null +++ b/spec/lib/banzai/reference_parser/snippet_parser_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::SnippetParser, lib: true do + include ReferenceParserHelpers + + let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } + let(:snippet) { create(:snippet, project: project) } + subject { described_class.new(project, user) } + let(:link) { empty_html_link } + + describe '#referenced_by' do + describe 'when the link has a data-snippet attribute' do + context 'using an existing snippet ID' do + it 'returns an Array of snippets' do + link['data-snippet'] = snippet.id.to_s + + expect(subject.referenced_by([link])).to eq([snippet]) + end + end + + context 'using a non-existing snippet ID' do + it 'returns an empty Array' do + link['data-snippet'] = '' + + expect(subject.referenced_by([link])).to eq([]) + end + end + end + end +end diff --git a/spec/lib/banzai/reference_parser/user_parser_spec.rb b/spec/lib/banzai/reference_parser/user_parser_spec.rb new file mode 100644 index 00000000000..9a82891297d --- /dev/null +++ b/spec/lib/banzai/reference_parser/user_parser_spec.rb @@ -0,0 +1,189 @@ +require 'spec_helper' + +describe Banzai::ReferenceParser::UserParser, lib: true do + include ReferenceParserHelpers + + let(:group) { create(:group) } + let(:user) { create(:user) } + let(:project) { create(:empty_project, :public, group: group, creator: user) } + subject { described_class.new(project, user) } + let(:link) { empty_html_link } + + describe '#referenced_by' do + context 'when the link has a data-group attribute' do + context 'using an existing group ID' do + before do + link['data-group'] = project.group.id.to_s + end + + it 'returns the users of the group' do + create(:group_member, group: group, user: user) + + expect(subject.referenced_by([link])).to eq([user]) + end + + it 'returns an empty Array when the group has no users' do + expect(subject.referenced_by([link])).to eq([]) + end + end + + context 'using a non-existing group ID' do + it 'returns an empty Array' do + link['data-group'] = '' + + expect(subject.referenced_by([link])).to eq([]) + end + end + end + + context 'when the link has a data-user attribute' do + it 'returns an Array of users' do + link['data-user'] = user.id.to_s + + expect(subject.referenced_by([link])).to eq([user]) + end + end + + context 'when the link has a data-project attribute' do + context 'using an existing project ID' do + let(:contributor) { create(:user) } + + before do + project.team << [user, :developer] + project.team << [contributor, :developer] + end + + it 'returns the members of a project' do + link['data-project'] = project.id.to_s + + # This uses an explicit sort to make sure this spec doesn't randomly + # fail when objects are returned in a different order. + refs = subject.referenced_by([link]).sort_by(&:id) + + expect(refs).to eq([user, contributor]) + end + end + + context 'using a non-existing project ID' do + it 'returns an empty Array' do + link['data-project'] = '' + + expect(subject.referenced_by([link])).to eq([]) + end + end + end + end + + describe '#nodes_visible_to_use?' do + context 'when the link has a data-group attribute' do + context 'using an existing group ID' do + before do + link['data-group'] = group.id.to_s + end + + it 'returns the nodes if the user can read the group' do + expect(Ability.abilities).to receive(:allowed?). + with(user, :read_group, group). + and_return(true) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + + it 'returns an empty Array if the user can not read the group' do + expect(Ability.abilities).to receive(:allowed?). + with(user, :read_group, group). + and_return(false) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([]) + end + end + + context 'when the link does not have a data-group attribute' do + context 'with a data-project attribute' do + it 'returns the nodes if the attribute value equals the current project ID' do + link['data-project'] = project.id.to_s + + expect(Ability.abilities).not_to receive(:allowed?) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + + it 'returns the nodes if the user can read the project' do + other_project = create(:empty_project, :public) + + link['data-project'] = other_project.id.to_s + + expect(Ability.abilities).to receive(:allowed?). + with(user, :read_project, other_project). + and_return(true) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + + it 'returns an empty Array if the user can not read the project' do + other_project = create(:empty_project, :public) + + link['data-project'] = other_project.id.to_s + + expect(Ability.abilities).to receive(:allowed?). + with(user, :read_project, other_project). + and_return(false) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([]) + end + end + + context 'without a data-project attribute' do + it 'returns the nodes' do + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + end + end + end + end + + describe '#nodes_user_can_reference' do + context 'when the link has a data-author attribute' do + it 'returns the nodes when the user is a member of the project' do + other_project = create(:project) + other_project.team << [user, :developer] + + link['data-project'] = other_project.id.to_s + link['data-author'] = user.id.to_s + + expect(subject.nodes_user_can_reference(user, [link])).to eq([link]) + end + + it 'returns an empty Array when the project could not be found' do + link['data-project'] = '' + link['data-author'] = user.id.to_s + + expect(subject.nodes_user_can_reference(user, [link])).to eq([]) + end + + it 'returns an empty Array when the user could not be found' do + other_project = create(:project) + + link['data-project'] = other_project.id.to_s + link['data-author'] = '' + + expect(subject.nodes_user_can_reference(user, [link])).to eq([]) + end + + it 'returns an empty Array when the user is not a team member' do + other_project = create(:project) + + link['data-project'] = other_project.id.to_s + link['data-author'] = user.id.to_s + + expect(subject.nodes_user_can_reference(user, [link])).to eq([]) + end + end + + context 'when the link does not have a data-author attribute' do + it 'returns the nodes' do + expect(subject.nodes_user_can_reference(user, [link])).to eq([link]) + end + end + end +end diff --git a/spec/lib/gitlab/lazy_spec.rb b/spec/lib/gitlab/lazy_spec.rb new file mode 100644 index 00000000000..b5ca89dd242 --- /dev/null +++ b/spec/lib/gitlab/lazy_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +describe Gitlab::Lazy, lib: true do + let(:dummy) { double(:dummy) } + + context 'when not calling any methods' do + it 'does not call the supplied block' do + expect(dummy).not_to receive(:foo) + + described_class.new { dummy.foo } + end + end + + context 'when calling a method on the object' do + it 'lazy loads the value returned by the block' do + expect(dummy).to receive(:foo).and_return('foo') + + lazy = described_class.new { dummy.foo } + + expect(lazy.to_s).to eq('foo') + end + end + + describe '#respond_to?' do + it 'returns true for a method defined on the wrapped object' do + lazy = described_class.new { 'foo' } + + expect(lazy).to respond_to(:downcase) + end + + it 'returns false for a method not defined on the wrapped object' do + lazy = described_class.new { 'foo' } + + expect(lazy).not_to respond_to(:quack) + end + end +end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb new file mode 100644 index 00000000000..1acb5846fcf --- /dev/null +++ b/spec/models/ability_spec.rb @@ -0,0 +1,117 @@ +require 'spec_helper' + +describe Ability, lib: true do + describe '.users_that_can_read_project' do + context 'using a public project' do + it 'returns all the users' do + project = create(:project, :public) + user = build(:user) + + expect(described_class.users_that_can_read_project([user], project)). + to eq([user]) + end + end + + context 'using an internal project' do + let(:project) { create(:project, :internal) } + + it 'returns users that are administrators' do + user = build(:user, admin: true) + + expect(described_class.users_that_can_read_project([user], project)). + to eq([user]) + end + + it 'returns internal users while skipping external users' do + user1 = build(:user) + user2 = build(:user, external: true) + users = [user1, user2] + + expect(described_class.users_that_can_read_project(users, project)). + to eq([user1]) + end + + it 'returns external users if they are the project owner' do + user1 = build(:user, external: true) + user2 = build(:user, external: true) + users = [user1, user2] + + expect(project).to receive(:owner).twice.and_return(user1) + + expect(described_class.users_that_can_read_project(users, project)). + to eq([user1]) + end + + it 'returns external users if they are project members' do + user1 = build(:user, external: true) + user2 = build(:user, external: true) + users = [user1, user2] + + expect(project.team).to receive(:members).twice.and_return([user1]) + + expect(described_class.users_that_can_read_project(users, project)). + to eq([user1]) + end + + it 'returns an empty Array if all users are external users without access' do + user1 = build(:user, external: true) + user2 = build(:user, external: true) + users = [user1, user2] + + expect(described_class.users_that_can_read_project(users, project)). + to eq([]) + end + end + + context 'using a private project' do + let(:project) { create(:project, :private) } + + it 'returns users that are administrators' do + user = build(:user, admin: true) + + expect(described_class.users_that_can_read_project([user], project)). + to eq([user]) + end + + it 'returns external users if they are the project owner' do + user1 = build(:user, external: true) + user2 = build(:user, external: true) + users = [user1, user2] + + expect(project).to receive(:owner).twice.and_return(user1) + + expect(described_class.users_that_can_read_project(users, project)). + to eq([user1]) + end + + it 'returns external users if they are project members' do + user1 = build(:user, external: true) + user2 = build(:user, external: true) + users = [user1, user2] + + expect(project.team).to receive(:members).twice.and_return([user1]) + + expect(described_class.users_that_can_read_project(users, project)). + to eq([user1]) + end + + it 'returns an empty Array if all users are internal users without access' do + user1 = build(:user) + user2 = build(:user) + users = [user1, user2] + + expect(described_class.users_that_can_read_project(users, project)). + to eq([]) + end + + it 'returns an empty Array if all users are external users without access' do + user1 = build(:user, external: true) + user2 = build(:user, external: true) + users = [user1, user2] + + expect(described_class.users_that_can_read_project(users, project)). + to eq([]) + end + end + end +end diff --git a/spec/models/commit_range_spec.rb b/spec/models/commit_range_spec.rb index 9307d97e214..6bc496414a3 100644 --- a/spec/models/commit_range_spec.rb +++ b/spec/models/commit_range_spec.rb @@ -24,6 +24,16 @@ describe CommitRange, models: true do expect { described_class.new("Foo", project) }.to raise_error(ArgumentError) end + describe '#initialize' do + it 'does not modify strings in-place' do + input = "#{sha_from}...#{sha_to} " + + described_class.new(input, project) + + expect(input).to eq("#{sha_from}...#{sha_to} ") + end + end + describe '#to_s' do it 'is correct for three-dot syntax' do expect(range.to_s).to eq "#{full_sha_from}...#{full_sha_to}" @@ -135,4 +145,27 @@ describe CommitRange, models: true do end end end + + describe '#has_been_reverted?' do + it 'returns true if the commit has been reverted' do + issue = create(:issue) + + create(:note_on_issue, + noteable_id: issue.id, + system: true, + note: commit1.revert_description) + + expect_any_instance_of(Commit).to receive(:reverts_commit?). + with(commit1). + and_return(true) + + expect(commit1.has_been_reverted?(nil, issue)).to eq(true) + end + + it 'returns false a commit has not been reverted' do + issue = create(:issue) + + expect(commit1.has_been_reverted?(nil, issue)).to eq(false) + end + end end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index ccb100cd96f..beca8708c9d 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Commit, models: true do - let(:project) { create(:project) } + let(:project) { create(:project, :public) } let(:commit) { project.commit } describe 'modules' do @@ -171,4 +171,40 @@ eos describe '#status' do # TODO: kamil end + + describe '#participants' do + let(:user1) { build(:user) } + let(:user2) { build(:user) } + + let!(:note1) do + create(:note_on_commit, + commit_id: commit.id, + project: project, + note: 'foo') + end + + let!(:note2) do + create(:note_on_commit, + commit_id: commit.id, + project: project, + note: 'bar') + end + + before do + allow(commit).to receive(:author).and_return(user1) + allow(commit).to receive(:committer).and_return(user2) + end + + it 'includes the commit author' do + expect(commit.participants).to include(commit.author) + end + + it 'includes the committer' do + expect(commit.participants).to include(commit.committer) + end + + it 'includes the authors of the commit notes' do + expect(commit.participants).to include(note1.author, note2.author) + end + end end diff --git a/spec/models/concerns/participable_spec.rb b/spec/models/concerns/participable_spec.rb new file mode 100644 index 00000000000..7e4ea0f2d66 --- /dev/null +++ b/spec/models/concerns/participable_spec.rb @@ -0,0 +1,83 @@ +require 'spec_helper' + +describe Participable, models: true do + let(:model) do + Class.new do + include Participable + end + end + + describe '.participant' do + it 'adds the participant attributes to the existing list' do + model.participant(:foo) + model.participant(:bar) + + expect(model.participant_attrs).to eq([:foo, :bar]) + end + end + + describe '#participants' do + it 'returns the list of participants' do + model.participant(:foo) + model.participant(:bar) + + user1 = build(:user) + user2 = build(:user) + user3 = build(:user) + project = build(:project, :public) + instance = model.new + + expect(instance).to receive(:foo).and_return(user2) + expect(instance).to receive(:bar).and_return(user3) + expect(instance).to receive(:project).twice.and_return(project) + + participants = instance.participants(user1) + + expect(participants).to include(user2) + expect(participants).to include(user3) + end + + it 'supports attributes returning another Participable' do + other_model = Class.new { include Participable } + + other_model.participant(:bar) + model.participant(:foo) + + instance = model.new + other = other_model.new + user1 = build(:user) + user2 = build(:user) + project = build(:project, :public) + + expect(instance).to receive(:foo).and_return(other) + expect(other).to receive(:bar).and_return(user2) + expect(instance).to receive(:project).twice.and_return(project) + + expect(instance.participants(user1)).to eq([user2]) + end + + context 'when using a Proc as an attribute' do + it 'calls the supplied Proc' do + user1 = build(:user) + project = build(:project, :public) + + user_arg = nil + ext_arg = nil + + model.participant -> (user, ext) do + user_arg = user + ext_arg = ext + end + + instance = model.new + + expect(instance).to receive(:project).twice.and_return(project) + + instance.participants(user1) + + expect(user_arg).to eq(user1) + expect(ext_arg).to be_an_instance_of(Gitlab::ReferenceExtractor) + end + end + end +end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 6540d77fbc0..87b3d8d650a 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -231,4 +231,42 @@ describe Issue, models: true do expect(issue.to_branch_name).to match /confidential-issue\z/ end end + + describe '#participants' do + context 'using a public project' do + let(:project) { create(:project, :public) } + let(:issue) { create(:issue, project: project) } + + let!(:note1) do + create(:note_on_issue, noteable: issue, project: project, note: 'a') + end + + let!(:note2) do + create(:note_on_issue, noteable: issue, project: project, note: 'b') + end + + it 'includes the issue author' do + expect(issue.participants).to include(issue.author) + end + + it 'includes the authors of the notes' do + expect(issue.participants).to include(note1.author, note2.author) + end + end + + context 'using a private project' do + it 'does not include mentioned users that do not have access to the project' do + project = create(:project) + user = create(:user) + issue = create(:issue, project: project) + + create(:note_on_issue, + noteable: issue, + project: project, + note: user.to_reference) + + expect(issue.participants).not_to include(user) + end + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 4b67c2facf3..118e1e22a78 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -414,4 +414,28 @@ describe MergeRequest, models: true do end end end + + describe '#participants' do + let(:project) { create(:project, :public) } + + let(:mr) do + create(:merge_request, source_project: project, target_project: project) + end + + let!(:note1) do + create(:note_on_merge_request, noteable: mr, project: project, note: 'a') + end + + let!(:note2) do + create(:note_on_merge_request, noteable: mr, project: project, note: 'b') + end + + it 'includes the merge request author' do + expect(mr.participants).to include(mr.author) + end + + it 'includes the authors of the notes' do + expect(mr.participants).to include(note1.author, note2.author) + end + end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 0e052907eec..b25150f7055 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -121,8 +121,19 @@ describe Note, models: true do let!(:note2) { create(:note_on_issue) } it "reads the rendered note body from the cache" do - expect(Banzai::Renderer).to receive(:render).with(note1.note, pipeline: :note, cache_key: [note1, "note"], project: note1.project) - expect(Banzai::Renderer).to receive(:render).with(note2.note, pipeline: :note, cache_key: [note2, "note"], project: note2.project) + expect(Banzai::Renderer).to receive(:render). + with(note1.note, + pipeline: :note, + cache_key: [note1, "note"], + project: note1.project, + author: note1.author) + + expect(Banzai::Renderer).to receive(:render). + with(note2.note, + pipeline: :note, + cache_key: [note2, "note"], + project: note2.project, + author: note2.author) note1.all_references note2.all_references @@ -248,4 +259,14 @@ describe Note, models: true do expect { note.valid? }.to change(note, :line_code).to(nil) end end + + describe '#participants' do + it 'includes the note author' do + project = create(:project, :public) + issue = create(:issue, project: project) + note = create(:note_on_issue, noteable: issue, project: project) + + expect(note.participants).to include(note.author) + end + end end diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index 7a613e360d4..789816bf2c7 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -87,4 +87,31 @@ describe Snippet, models: true do expect(described_class.search_code('FOO')).to eq([snippet]) end end + + describe '#participants' do + let(:project) { create(:project, :public) } + let(:snippet) { create(:snippet, content: 'foo', project: project) } + + let!(:note1) do + create(:note_on_project_snippet, + noteable: snippet, + project: project, + note: 'a') + end + + let!(:note2) do + create(:note_on_project_snippet, + noteable: snippet, + project: project, + note: 'b') + end + + it 'includes the snippet author' do + expect(snippet.participants).to include(snippet.author) + end + + it 'includes the note authors' do + expect(snippet.participants).to include(note1.author, note2.author) + end + end end diff --git a/spec/support/filter_spec_helper.rb b/spec/support/filter_spec_helper.rb index e849a9633b9..a8e454eb09e 100644 --- a/spec/support/filter_spec_helper.rb +++ b/spec/support/filter_spec_helper.rb @@ -40,8 +40,7 @@ module FilterSpecHelper filters = [ Banzai::Filter::AutolinkFilter, - described_class, - Banzai::Filter::ReferenceGathererFilter + described_class ] HTML::Pipeline.new(filters, context) diff --git a/spec/support/reference_parser_helpers.rb b/spec/support/reference_parser_helpers.rb new file mode 100644 index 00000000000..01689194eac --- /dev/null +++ b/spec/support/reference_parser_helpers.rb @@ -0,0 +1,5 @@ +module ReferenceParserHelpers + def empty_html_link + Nokogiri::HTML.fragment('<a></a>').children[0] + end +end |