BigW Consortium Gitlab

Skip to content

G
gitlab-ce
Commit 0c2fdb1c by Jarka Kadlecova
Options

Refactor banzai to support referencing from group context

parent 50eb1252
Showing with 273 additions and 166 deletions
app/helpers/markup_helper.rb
......@@ -86,6 +86,8 @@ module MarkupHelper
return '' unless text.present?
context[:project] ||= @project
context[:group] ||= @group
html = markdown_unsafe(text, context)
prepare_for_rendering(html, context)
end
......
app/models/epic.rb
# Placeholder class for model that is implemented in EE
# It will reserve (ee#3853) '&' as a reference prefix, but the table does not exists in CE
# It reserves '&' as a reference prefix, but the table does not exists in CE
class Epic < ActiveRecord::Base
# TODO: this will be implemented as part of #3853
def to_reference
def self.reference_prefix
'&'
end
def self.reference_prefix_escaped
'&amp;'
end
end
lib/banzai/cross_project_reference.rb
......@@ -11,7 +11,7 @@ module Banzai
# ref - String reference.
#
# Returns a Project, or nil if the reference can't be found
def project_from_ref(ref)
def parent_from_ref(ref)
return context[:project] unless ref
Project.find_by_full_path(ref)
......
lib/banzai/filter/abstract_reference_filter.rb
......@@ -82,9 +82,9 @@ module Banzai
end
end
def project_from_ref_cached(ref)
cached_call(:banzai_project_refs, ref) do
project_from_ref(ref)
def from_ref_cached(ref)
cached_call("banzai_#{parent_type}_refs".to_sym, ref) do
parent_from_ref(ref)
end
end
......@@ -153,15 +153,20 @@ module Banzai
# have `gfm` and `gfm-OBJECT_NAME` class names attached for styling.
def object_link_filter(text, pattern, link_content: nil, link_reference: false)
references_in(text, pattern) do |match, id, project_ref, namespace_ref, matches|
project_path = full_project_path(namespace_ref, project_ref)
project = project_from_ref_cached(project_path)
parent_path = if parent_type == :group
full_group_path(namespace_ref)
else
full_project_path(namespace_ref, project_ref)
end
if project
parent = from_ref_cached(parent_path)
if parent
object =
if link_reference
find_object_from_link_cached(project, id)
find_object_from_link_cached(parent, id)
else
find_object_cached(project, id)
find_object_cached(parent, id)
end
end
......@@ -169,13 +174,13 @@ module Banzai
title = object_link_title(object)
klass = reference_class(object_sym)
data = data_attributes_for(link_content || match, project, object, link: !!link_content)
data = data_attributes_for(link_content || match, parent, object, link: !!link_content)
url =
if matches.names.include?("url") && matches[:url]
matches[:url]
else
url_for_object_cached(object, project)
url_for_object_cached(object, parent)
end
content = link_content || object_link_text(object, matches)
......@@ -224,17 +229,24 @@ module Banzai
# Returns a Hash containing all object references (e.g. issue IDs) per the
# project they belong to.
def references_per_project
@references_per_project ||= begin
def references_per_parent
@references_per ||= {}
@references_per[parent_type] ||= begin
refs = Hash.new { |hash, key| hash[key] = Set.new }
regex = Regexp.union(object_class.reference_pattern, object_class.link_reference_pattern)
nodes.each do |node|
node.to_html.scan(regex) do
project_path = full_project_path($~[:namespace], $~[:project])
path = if parent_type == :project
full_project_path($~[:namespace], $~[:project])
else
full_group_path($~[:group])
end
symbol = $~[object_sym]
refs[project_path] << symbol if object_class.reference_valid?(symbol)
refs[path] << symbol if object_class.reference_valid?(symbol)
end
end
......@@ -244,35 +256,41 @@ module Banzai
# Returns a Hash containing referenced projects grouped per their full
# path.
def projects_per_reference
@projects_per_reference ||= begin
def parent_per_reference
@per_reference ||= {}
@per_reference[parent_type] ||= begin
refs = Set.new
references_per_project.each do |project_ref, _|
refs << project_ref
references_per_parent.each do |ref, _|
refs << ref
end
find_projects_for_paths(refs.to_a).index_by(&:full_path)
find_for_paths(refs.to_a).index_by(&:full_path)
end
end
def projects_relation_for_paths(paths)
Project.where_full_path_in(paths).includes(:namespace)
def relation_for_paths(paths)
klass = parent_type.to_s.camelize.constantize
result = klass.where_full_path_in(paths)
return result if parent_type == :group
result.includes(:namespace) if parent_type == :project
end
# Returns projects for the given paths.
def find_projects_for_paths(paths)
def find_for_paths(paths)
if RequestStore.active?
cache = project_refs_cache
cache = refs_cache
to_query = paths - cache.keys
unless to_query.empty?
projects = projects_relation_for_paths(to_query)
records = relation_for_paths(to_query)
found = []
projects.each do |project|
ref = project.full_path
get_or_set_cache(cache, ref) { project }
records.each do |record|
ref = record.full_path
get_or_set_cache(cache, ref) { record }
found << ref
end
......@@ -284,33 +302,37 @@ module Banzai
cache.slice(*paths).values.compact
else
projects_relation_for_paths(paths)
relation_for_paths(paths)
end
end
def current_project_path
return unless project
@current_project_path ||= project.full_path
def current_parent_path
@current_parent_path ||= parent&.full_path
end
def current_project_namespace_path
return unless project
@current_project_namespace_path ||= project.namespace.full_path
@current_project_namespace_path ||= project&.namespace&.full_path
end
private
def full_project_path(namespace, project_ref)
return current_project_path unless project_ref
return current_parent_path unless project_ref
namespace_ref = namespace || current_project_namespace_path
"#{namespace_ref}/#{project_ref}"
end
def project_refs_cache
RequestStore[:banzai_project_refs] ||= {}
def refs_cache
RequestStore["banzai_#{parent_type}_refs".to_sym] ||= {}
end
def parent_type
:project
end
def parent
parent_type == :project ? project : group
end
end
end
......
lib/banzai/filter/epic_reference_filter.rb 0 → 100644
module Banzai
module Filter
# The actual filter is implemented in the EE mixin
class EpicReferenceFilter < IssuableReferenceFilter
self.reference_type = :epic
def self.object_class
Epic
end
end
end
end
lib/banzai/filter/issuable_reference_filter.rb 0 → 100644
module Banzai
module Filter
class IssuableReferenceFilter < AbstractReferenceFilter
def records_per_parent
@records_per_project ||= {}
@records_per_project[object_class.to_s.underscore] ||= begin
hash = Hash.new { |h, k| h[k] = {} }
parent_per_reference.each do |path, parent|
record_ids = references_per_parent[path]
parent_records(parent, record_ids).each do |record|
hash[parent][record.iid.to_i] = record
end
end
hash
end
end
def find_object(parent, iid)
records_per_parent[parent][iid]
end
def parent_from_ref(ref)
parent_per_reference[ref || current_parent_path]
end
end
end
end
lib/banzai/filter/issue_reference_filter.rb
......@@ -8,46 +8,24 @@ module Banzai
# When external issues tracker like Jira is activated we should not
# use issue reference pattern, but we should still be able
# to reference issues from other GitLab projects.
class IssueReferenceFilter < AbstractReferenceFilter
class IssueReferenceFilter < IssuableReferenceFilter
self.reference_type = :issue
def self.object_class
Issue
end
def find_object(project, iid)
issues_per_project[project][iid]
end
def url_for_object(issue, project)
IssuesHelper.url_for_issue(issue.iid, project, only_path: context[:only_path], internal: true)
end
def project_from_ref(ref)
projects_per_reference[ref || current_project_path]
end
# Returns a Hash containing the issues per Project instance.
def issues_per_project
@issues_per_project ||= begin
hash = Hash.new { |h, k| h[k] = {} }
projects_per_reference.each do |path, project|
issue_ids = references_per_project[path]
issues = project.issues.where(iid: issue_ids.to_a)
issues.each do |issue|
hash[project][issue.iid.to_i] = issue
end
end
hash
end
end
def projects_relation_for_paths(paths)
super(paths).includes(:gitlab_issue_tracker_service)
end
def parent_records(parent, ids)
parent.issues.where(iid: ids.to_a)
end
end
end
end
lib/banzai/filter/label_reference_filter.rb
......@@ -33,7 +33,7 @@ module Banzai
end
def find_label(project_ref, label_id, label_name)
project = project_from_ref(project_ref)
project = parent_from_ref(project_ref)
return unless project
label_params = label_params(label_id, label_name)
......@@ -66,7 +66,7 @@ module Banzai
def object_link_text(object, matches)
project_path = full_project_path(matches[:namespace], matches[:project])
project_from_ref = project_from_ref_cached(project_path)
project_from_ref = from_ref_cached(project_path)
reference = project_from_ref.to_human_reference(project)
label_suffix = " <i>in #{reference}</i>" if reference.present?
......
lib/banzai/filter/merge_request_reference_filter.rb
......@@ -4,48 +4,19 @@ module Banzai
# to merge requests that do not exist are ignored.
#
# This filter supports cross-project references.
class MergeRequestReferenceFilter < AbstractReferenceFilter
class MergeRequestReferenceFilter < IssuableReferenceFilter
self.reference_type = :merge_request
def self.object_class
MergeRequest
end
def find_object(project, iid)
merge_requests_per_project[project][iid]
end
def url_for_object(mr, project)
h = Gitlab::Routing.url_helpers
h.project_merge_request_url(project, mr,
only_path: context[:only_path])
end
def project_from_ref(ref)
projects_per_reference[ref || current_project_path]
end
# Returns a Hash containing the merge_requests per Project instance.
def merge_requests_per_project
@merge_requests_per_project ||= begin
hash = Hash.new { |h, k| h[k] = {} }
projects_per_reference.each do |path, project|
merge_request_ids = references_per_project[path]
merge_requests = project.merge_requests
.where(iid: merge_request_ids.to_a)
.includes(target_project: :namespace)
merge_requests.each do |merge_request|
hash[project][merge_request.iid.to_i] = merge_request
end
end
hash
end
end
def object_link_text_extras(object, matches)
extras = super
......@@ -61,6 +32,12 @@ module Banzai
extras
end
def parent_records(parent, ids)
parent.merge_requests
.where(iid: ids.to_a)
.includes(target_project: :namespace)
end
end
end
end
lib/banzai/filter/milestone_reference_filter.rb
......@@ -38,7 +38,7 @@ module Banzai
def find_milestone(project_ref, namespace_ref, milestone_id, milestone_name)
project_path = full_project_path(namespace_ref, project_ref)
project = project_from_ref(project_path)
project = parent_from_ref(project_path)
return unless project
......
lib/banzai/issuable_extractor.rb
......@@ -28,8 +28,8 @@ module Banzai
issue_parser = Banzai::ReferenceParser::IssueParser.new(project, user)
merge_request_parser = Banzai::ReferenceParser::MergeRequestParser.new(project, user)
issuables_for_nodes = issue_parser.issues_for_nodes(nodes).merge(
merge_request_parser.merge_requests_for_nodes(nodes)
issuables_for_nodes = issue_parser.records_for_nodes(nodes).merge(
merge_request_parser.records_for_nodes(nodes)
)
# The project for the issue/MR might be pending for deletion!
......
lib/banzai/reference_parser/epic_parser.rb 0 → 100644
module Banzai
module ReferenceParser
# The actual parser is implemented in the EE mixin
class EpicParser < IssuableParser
self.reference_type = :epic
def records_for_nodes(_nodes)
{}
end
end
end
end
lib/banzai/reference_parser/issuable_parser.rb 0 → 100644
module Banzai
module ReferenceParser
class IssuableParser < BaseParser
def nodes_visible_to_user(user, nodes)
records = records_for_nodes(nodes)
nodes.select do |node|
issuable = records[node]
issuable && can_read_reference?(user, issuable)
end
end
def referenced_by(nodes)
records = records_for_nodes(nodes)
nodes.map { |node| records[node] }.compact.uniq
end
def can_read_reference?(user, issuable)
can?(user, "read_#{issuable.class.to_s.underscore}".to_sym, issuable)
end
end
end
end
lib/banzai/reference_parser/issue_parser.rb
module Banzai
module ReferenceParser
class IssueParser < BaseParser
class IssueParser < IssuableParser
self.reference_type = :issue
def nodes_visible_to_user(user, nodes)
issues = issues_for_nodes(nodes)
issues = records_for_nodes(nodes)
readable_issues = Ability
.issues_readable_by_user(issues.values, user).to_set
......@@ -14,13 +14,7 @@ module Banzai
end
end
def referenced_by(nodes)
issues = issues_for_nodes(nodes)
nodes.map { |node| issues[node] }.compact.uniq
end
def issues_for_nodes(nodes)
def records_for_nodes(nodes)
@issues_for_nodes ||= grouped_objects_for_nodes(
nodes,
Issue.all.includes(
......
lib/banzai/reference_parser/merge_request_parser.rb
module Banzai
module ReferenceParser
class MergeRequestParser < BaseParser
class MergeRequestParser < IssuableParser
self.reference_type = :merge_request
def nodes_visible_to_user(user, nodes)
merge_requests = merge_requests_for_nodes(nodes)
nodes.select do |node|
merge_request = merge_requests[node]
merge_request && can?(user, :read_merge_request, merge_request.project)
end
end
def referenced_by(nodes)
merge_requests = merge_requests_for_nodes(nodes)
nodes.map { |node| merge_requests[node] }.compact.uniq
end
def merge_requests_for_nodes(nodes)
def records_for_nodes(nodes)
@merge_requests_for_nodes ||= grouped_objects_for_nodes(
nodes,
MergeRequest.includes(
......@@ -40,10 +24,6 @@ module Banzai
self.class.data_attribute
)
end
def can_read_reference?(user, ref_project, node)
can?(user, :read_merge_request, ref_project)
end
end
end
end
lib/gitlab/reference_extractor.rb
module Gitlab
# Extract possible GFM references from an arbitrary String for further processing.
class ReferenceExtractor < Banzai::ReferenceExtractor
REFERABLES = %i(user issue label milestone merge_request snippet commit commit_range directly_addressed_user).freeze
REFERABLES = %i(user issue label milestone merge_request snippet commit commit_range directly_addressed_user epic).freeze
attr_accessor :project, :current_user, :author
def initialize(project, current_user = nil)
......
spec/features/markdown_spec.rb
......@@ -207,8 +207,9 @@ describe 'GitLab Markdown' do
before do
@feat = MarkdownFeature.new
# `markdown` helper expects a `@project` variable
# `markdown` helper expects a `@project` and `@group` variable
@project = @feat.project
@group = @feat.group
end
context 'default pipeline' do
......
spec/lib/banzai/cross_project_reference_spec.rb
......@@ -3,20 +3,20 @@ require 'spec_helper'
describe Banzai::CrossProjectReference do
include described_class
describe '#project_from_ref' do
describe '#parent_from_ref' do
context 'when no project was referenced' do
it 'returns the project from context' do
project = double
allow(self).to receive(:context).and_return({ project: project })
expect(project_from_ref(nil)).to eq project
expect(parent_from_ref(nil)).to eq project
end
end
context 'when referenced project does not exist' do
it 'returns nil' do
expect(project_from_ref('invalid/reference')).to be_nil
expect(parent_from_ref('invalid/reference')).to be_nil
end
end
......@@ -27,7 +27,7 @@ describe Banzai::CrossProjectReference do
expect(Project).to receive(:find_by_full_path)
.with('cross/reference').and_return(project2)
expect(project_from_ref('cross/reference')).to eq project2
expect(parent_from_ref('cross/reference')).to eq project2
end
end
end
......
spec/lib/banzai/filter/abstract_reference_filter_spec.rb
......@@ -3,67 +3,67 @@ require 'spec_helper'
describe Banzai::Filter::AbstractReferenceFilter do
let(:project) { create(:project) }
describe '#references_per_project' do
it 'returns a Hash containing references grouped per project paths' do
describe '#references_per_parent' do
it 'returns a Hash containing references grouped per parent paths' do
doc = Nokogiri::HTML.fragment("#1 #{project.full_path}#2")
filter = described_class.new(doc, project: project)
expect(filter).to receive(:object_class).exactly(4).times.and_return(Issue)
expect(filter).to receive(:object_sym).twice.and_return(:issue)
refs = filter.references_per_project
refs = filter.references_per_parent
expect(refs).to be_an_instance_of(Hash)
expect(refs[project.full_path]).to eq(Set.new(%w[1 2]))
end
end
describe '#projects_per_reference' do
it 'returns a Hash containing projects grouped per project paths' do
describe '#parent_per_reference' do
it 'returns a Hash containing projects grouped per parent paths' do
doc = Nokogiri::HTML.fragment('')
filter = described_class.new(doc, project: project)
expect(filter).to receive(:references_per_project)
expect(filter).to receive(:references_per_parent)
.and_return({ project.full_path => Set.new(%w[1]) })
expect(filter.projects_per_reference)
expect(filter.parent_per_reference)
.to eq({ project.full_path => project })
end
end
describe '#find_projects_for_paths' do
describe '#find_for_paths' do
let(:doc) { Nokogiri::HTML.fragment('') }
let(:filter) { described_class.new(doc, project: project) }
context 'with RequestStore disabled' do
it 'returns a list of Projects for a list of paths' do
expect(filter.find_projects_for_paths([project.full_path]))
expect(filter.find_for_paths([project.full_path]))
.to eq([project])
end
it "return an empty array for paths that don't exist" do
expect(filter.find_projects_for_paths(['nonexistent/project']))
expect(filter.find_for_paths(['nonexistent/project']))
.to eq([])
end
end
context 'with RequestStore enabled', :request_store do
it 'returns a list of Projects for a list of paths' do
expect(filter.find_projects_for_paths([project.full_path]))
expect(filter.find_for_paths([project.full_path]))
.to eq([project])
end
context "when no project with that path exists" do
it "returns no value" do
expect(filter.find_projects_for_paths(['nonexistent/project']))
expect(filter.find_for_paths(['nonexistent/project']))
.to eq([])
end
it "adds the ref to the project refs cache" do
project_refs_cache = {}
allow(filter).to receive(:project_refs_cache).and_return(project_refs_cache)
allow(filter).to receive(:refs_cache).and_return(project_refs_cache)
filter.find_projects_for_paths(['nonexistent/project'])
filter.find_for_paths(['nonexistent/project'])
expect(project_refs_cache).to eq({ 'nonexistent/project' => nil })
end
......@@ -71,11 +71,11 @@ describe Banzai::Filter::AbstractReferenceFilter do
context 'when the project refs cache includes nil values' do
before do
# adds { 'nonexistent/project' => nil } to cache
filter.project_from_ref_cached('nonexistent/project')
filter.from_ref_cached('nonexistent/project')
end
it "return an empty array for paths that don't exist" do
expect(filter.find_projects_for_paths(['nonexistent/project']))
expect(filter.find_for_paths(['nonexistent/project']))
.to eq([])
end
end
......@@ -83,12 +83,12 @@ describe Banzai::Filter::AbstractReferenceFilter do
end
end
describe '#current_project_path' do
it 'returns the path of the current project' do
describe '#current_parent_path' do
it 'returns the path of the current parent' do
doc = Nokogiri::HTML.fragment('')
filter = described_class.new(doc, project: project)
expect(filter.current_project_path).to eq(project.full_path)
expect(filter.current_parent_path).to eq(project.full_path)
end
end
end
spec/lib/banzai/filter/issue_reference_filter_spec.rb
......@@ -157,6 +157,12 @@ describe Banzai::Filter::IssueReferenceFilter do
expect(doc.text).to eq("Fixed (#{project2.full_path}##{issue.iid}.)")
end
it 'includes default classes' do
doc = reference_filter("Fixed (#{reference}.)")
expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-issue has-tooltip'
end
it 'ignores invalid issue IDs on the referenced project' do
exp = act = "Fixed #{invalidate_reference(reference)}"
......@@ -201,6 +207,12 @@ describe Banzai::Filter::IssueReferenceFilter do
expect(doc.text).to eq("Fixed (#{project2.path}##{issue.iid}.)")
end
it 'includes default classes' do
doc = reference_filter("Fixed (#{reference}.)")
expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-issue has-tooltip'
end
it 'ignores invalid issue IDs on the referenced project' do
exp = act = "Fixed #{invalidate_reference(reference)}"
......@@ -245,6 +257,12 @@ describe Banzai::Filter::IssueReferenceFilter do
expect(doc.text).to eq("Fixed (#{project2.path}##{issue.iid}.)")
end
it 'includes default classes' do
doc = reference_filter("Fixed (#{reference}.)")
expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-issue has-tooltip'
end
it 'ignores invalid issue IDs on the referenced project' do
exp = act = "Fixed #{invalidate_reference(reference)}"
......@@ -269,8 +287,15 @@ describe Banzai::Filter::IssueReferenceFilter do
it 'links with adjacent text' do
doc = reference_filter("Fixed (#{reference}.)")
expect(doc.to_html).to match(/\(<a.+>#{Regexp.escape(issue.to_reference(project))} \(comment 123\)<\/a>\.\)/)
end
it 'includes default classes' do
doc = reference_filter("Fixed (#{reference}.)")
expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-issue has-tooltip'
end
end
context 'cross-project reference in link href' do
......@@ -291,8 +316,15 @@ describe Banzai::Filter::IssueReferenceFilter do
it 'links with adjacent text' do
doc = reference_filter("Fixed (#{reference_link}.)")
expect(doc.to_html).to match(/\(<a.+>Reference<\/a>\.\)/)
end
it 'includes default classes' do
doc = reference_filter("Fixed (#{reference_link}.)")
expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-issue has-tooltip'
end
end
context 'cross-project URL in link href' do
......@@ -313,8 +345,15 @@ describe Banzai::Filter::IssueReferenceFilter do
it 'links with adjacent text' do
doc = reference_filter("Fixed (#{reference_link}.)")
expect(doc.to_html).to match(/\(<a.+>Reference<\/a>\.\)/)
end
it 'includes default classes' do
doc = reference_filter("Fixed (#{reference_link}.)")
expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-issue has-tooltip'
end
end
context 'group context' do
......@@ -387,19 +426,19 @@ describe Banzai::Filter::IssueReferenceFilter do
end
end
describe '#issues_per_project' do
describe '#records_per_parent' do
context 'using an internal issue tracker' do
it 'returns a Hash containing the issues per project' do
doc = Nokogiri::HTML.fragment('')
filter = described_class.new(doc, project: project)
expect(filter).to receive(:projects_per_reference)
expect(filter).to receive(:parent_per_reference)
.and_return({ project.full_path => project })
expect(filter).to receive(:references_per_project)
expect(filter).to receive(:references_per_parent)
.and_return({ project.full_path => Set.new([issue.iid]) })
expect(filter.issues_per_project)
expect(filter.records_per_parent)
.to eq({ project => { issue.iid => issue } })
end
end
......
spec/lib/banzai/reference_parser/issue_parser_spec.rb
......@@ -70,12 +70,12 @@ describe Banzai::ReferenceParser::IssueParser do
end
end
describe '#issues_for_nodes' do
describe '#records_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({ link => issue })
expect(subject.records_for_nodes(nodes)).to eq({ link => issue })
end
end
end
spec/lib/gitlab/reference_extractor_spec.rb
......@@ -250,4 +250,34 @@ describe Gitlab::ReferenceExtractor do
subject { described_class.references_pattern }
it { is_expected.to be_kind_of Regexp }
end
describe 'referables prefixes' do
def prefixes
described_class::REFERABLES.each_with_object({}) do |referable, result|
klass = referable.to_s.camelize.constantize
next unless klass.respond_to?(:reference_prefix)
prefix = klass.reference_prefix
result[prefix] ||= []
result[prefix] << referable
end
end
it 'returns all supported prefixes' do
expect(prefixes.keys.uniq).to match_array(%w(@ # ~ % ! $ &))
end
it 'does not allow one prefix for multiple referables if not allowed specificly' do
# make sure you are not overriding existing prefix before changing this hash
multiple_allowed = {
'@' => 3
}
prefixes.each do |prefix, referables|
expected_count = multiple_allowed[prefix] || 1
expect(referables.count).to eq(expected_count)
end
end
end
end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment