diff --git a/app/helpers/mail_formatting_helper.rb b/app/helpers/mail_formatting_helper.rb new file mode 100644 index 00000000000..545da16ca2c --- /dev/null +++ b/app/helpers/mail_formatting_helper.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +# Pin the external rendering channel so mailer templates never have to +# remember the `render_mode:` / `only_path:` / `static_html:` combination. +# Matching the `.html.erb` / `.text.erb` extension to the helper name keeps +# caller intent visible. +module MailFormattingHelper + def format_mail_html(*, **) + format_text(*, render_mode: :external_html, **) + end + + def format_mail_text(*, **) + format_text(*, render_mode: :external_text, **) + end +end diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 70ac30581dd..4912ae22742 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -34,6 +34,7 @@ class ApplicationMailer < ActionMailer::Base helper :application, # for format_text :work_packages, # for css classes :custom_fields, # for show_value + :mail_formatting, # for format_mail_html / format_mail_text :mail_layout # for layouting include OpenProject::LocaleHelper diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index b743f058b42..43c9769d59e 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -121,6 +121,13 @@ module WorkPackage::SemanticIdentifier end end + # Returns formatted value for inline UI display. + # * Semantic mode: "PROJ-42" (no prefix — self-describing) + # * Classic mode: "#42" (hash-prefixed) + def self.format_display_id(display_id) + display_id.is_a?(String) && display_id.match?(/[A-Za-z]/) ? display_id : "##{display_id}" + end + # Returns the user-facing identifier for this work package. # In semantic mode: the project-based identifier (e.g. "PROJ-42") # In classic mode: the numeric database ID @@ -134,8 +141,7 @@ module WorkPackage::SemanticIdentifier # Semantic mode: "PROJ-42" (no prefix — self-describing) # Classic mode: "#42" (hash-prefixed) def formatted_id - did = display_id - did.is_a?(String) && did.match?(/[A-Za-z]/) ? did : "##{did}" + WorkPackage::SemanticIdentifier.format_display_id(display_id) end # Override ActiveRecord's default `to_param` so Rails URL helpers diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index 7f4ac6c0d67..9466248c8bb 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -123,7 +123,8 @@ class WorkPackages::UpdateAncestorsService < BaseServices::BaseCallable def set_journal_note(work_packages) work_packages.each do |wp| - wp.journal_notes = I18n.t("work_package.updated_automatically_by_child_changes", child: "##{initiator_work_package.id}") + wp.journal_notes = I18n.t("work_package.updated_automatically_by_child_changes", + child: "##{initiator_work_package.id}") end end diff --git a/app/views/work_package_mailer/_work_package_details.html.erb b/app/views/work_package_mailer/_work_package_details.html.erb index 81b1173dd06..626d8f6210e 100644 --- a/app/views/work_package_mailer/_work_package_details.html.erb +++ b/app/views/work_package_mailer/_work_package_details.html.erb @@ -47,4 +47,4 @@ See COPYRIGHT and LICENSE files for more details. <% end %> -<%= format_text(work_package, :description, only_path: false) %> +<%= format_mail_html(work_package, :description) %> diff --git a/app/views/work_package_mailer/mentioned.html.erb b/app/views/work_package_mailer/mentioned.html.erb index 69831d69f34..a7b6d914862 100644 --- a/app/views/work_package_mailer/mentioned.html.erb +++ b/app/views/work_package_mailer/mentioned.html.erb @@ -26,7 +26,7 @@ >
- <%= format_text @journal.notes, only_path: false %> + <%= format_mail_html @journal.notes %>
diff --git a/app/views/work_package_mailer/mentioned.text.erb b/app/views/work_package_mailer/mentioned.text.erb index eee14ad5c8b..f93db4e5806 100644 --- a/app/views/work_package_mailer/mentioned.text.erb +++ b/app/views/work_package_mailer/mentioned.text.erb @@ -8,6 +8,10 @@ <%= "=" * ((@work_package.formatted_id + " " + @work_package.subject).length + 4) %> <%= I18n.t(:label_comment_added) %>: -<%= strip_tags @journal.notes %> +<%= format_mail_text( + @journal.notes, + object: @work_package, + project: @work_package.project + ) %> <%= "-" * 100 %> diff --git a/app/views/work_package_mailer/watcher_changed.html.erb b/app/views/work_package_mailer/watcher_changed.html.erb index 6089c2dd7b9..bd169b9be43 100644 --- a/app/views/work_package_mailer/watcher_changed.html.erb +++ b/app/views/work_package_mailer/watcher_changed.html.erb @@ -30,9 +30,8 @@ See COPYRIGHT and LICENSE files for more details.
<%= render partial: "work_package_details", locals: { work_package: @work_package } %>

- <%= format_text( + <%= format_mail_html( t(:text_latest_note, note: last_work_package_note(@work_package)), - only_path: false, object: @work_package, project: @work_package.project ) %> diff --git a/app/views/work_package_mailer/watcher_changed.text.erb b/app/views/work_package_mailer/watcher_changed.text.erb index 7de3fc8bccc..841f65c0fe3 100644 --- a/app/views/work_package_mailer/watcher_changed.text.erb +++ b/app/views/work_package_mailer/watcher_changed.text.erb @@ -31,4 +31,8 @@ See COPYRIGHT and LICENSE files for more details. ---------------------------------------- <%= render partial: "work_package_details", locals: { work_package: @work_package } %> -<%= t(:text_latest_note, note: last_work_package_note(@work_package)) %> +<%= format_mail_text( + t(:text_latest_note, note: last_work_package_note(@work_package)), + object: @work_package, + project: @work_package.project + ) %> diff --git a/lib/open_project/text_formatting.rb b/lib/open_project/text_formatting.rb index c46d9ffcbfe..57b9251ab57 100644 --- a/lib/open_project/text_formatting.rb +++ b/lib/open_project/text_formatting.rb @@ -43,20 +43,25 @@ module OpenProject # @!macro format_text_params # @param [Project] project a Project context. - # @param [Boolean] only_path whether to generate links with relative URLs. + # @param [Symbol] render_mode the rendering channel (`:in_app_html`, + # `:external_html`, `:external_text`). Resolves the `only_path`, + # `static_html` and `plain_text` context flags as a set. Prefer this + # over passing the primitives individually. See {RenderMode}. + # @param [Boolean] only_path explicit override for the resolved `only_path`. # @param [User] current_user the current user context. # @param [:plain, :rich] format the text format. # `:plain` will return plain text. # `:rich` will render raw Markdown as HTML. # @param ** [Hash] additional context to pass to the underlying rendering - # pipeline. + # pipeline. Explicit `static_html:` / `plain_text:` here override the + # values resolved from `render_mode:`. # rubocop:disable Layout/LineLength ## # Formats text according to system settings and provided params. # - # @overload format_text(text, object: nil, project: @project || object.try(:project), only_path: true, current_user: User.current, format: :rich, **) + # @overload format_text(text, object: nil, project: @project || object.try(:project), render_mode: :in_app_html, only_path: nil, current_user: User.current, format: :rich, **) # # @param [String] text the raw text to be formatted, typically Markdown. # @param [Object] object an object context. @@ -64,10 +69,10 @@ module OpenProject # # @example Setting a project context explicitly # format_text("## Hello world", project: current_project) - # @example Generating links with full URLs - # format_text("[Projects](/projects)", only_path: false) + # @example Rendering for an external surface (mailer, RSS, export) + # format_text("see #42", render_mode: :external_html) # - # @overload format_text(object, attribute, project: @project || object.try(:project), only_path: true, current_user: User.current, format: :rich, **) + # @overload format_text(object, attribute, project: @project || object.try(:project), render_mode: :in_app_html, only_path: nil, current_user: User.current, format: :rich, **) # # @param [Object] object an object, typically a model # (i.e. `ActiveRecord::Base` descendent). @@ -79,7 +84,8 @@ module OpenProject # format_text(issue, :description, options) # # @return [String] the formatted text as an HTML-safe String. - def format_text(*args, object: nil, project: nil, only_path: true, current_user: User.current, format: :rich, **) + def format_text(*args, object: nil, project: nil, render_mode: :in_app_html, + only_path: nil, current_user: User.current, format: :rich, **kwargs) case args.size when 1 attribute = nil @@ -94,15 +100,22 @@ module OpenProject project ||= @project || object.try(:project) + resolved = RenderMode.resolve( + render_mode, + only_path:, + static_html: kwargs.delete(:static_html), + plain_text: kwargs.delete(:plain_text) + ) + Renderer.format_text( text, - **, + **kwargs, format:, object:, request: try(:request), current_user:, attribute:, - only_path:, + **resolved, project: ) end diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 4563289e07b..f1c8defddda 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -37,6 +37,8 @@ module OpenProject::TextFormatting include OpenProject::StaticRouting::UrlHelpers def call + preload_mentions + doc.search("mention").each do |mention| anchor = mention_anchor(mention) mention.replace(anchor) if anchor @@ -47,6 +49,41 @@ module OpenProject::TextFormatting private + # WP labels resolve regardless of viewer (so an inaccessible WP + # still renders its current formatted_id); a separate id pluck + # gates anchor-vs-text. Principals collapse the two concerns into + # one visibility-scoped fetch — invisible users and groups fall + # back to the literal envelope text. + def preload_mentions + preload_work_package_mentions + preload_principal_mentions + end + + def preload_work_package_mentions + ids = mention_ids_for("work_package") + if ids.empty? + @mentioned_work_packages = {} + @visible_mentioned_ids = Set.new + return + end + + scope = WorkPackage.where(id: ids) + scope = scope.includes(:type, :status) if context[:static_html] + @mentioned_work_packages = scope.index_by(&:id) + @visible_mentioned_ids = WorkPackage.visible.where(id: ids).pluck(:id).to_set + end + + def preload_principal_mentions + user_ids = mention_ids_for("user") + group_ids = mention_ids_for("group") + @mentioned_users = user_ids.empty? ? {} : User.visible.where(id: user_ids).index_by(&:id) + @mentioned_groups = group_ids.empty? ? {} : Group.visible.where(id: group_ids).index_by(&:id) + end + + def mention_ids_for(type) + doc.css(%(mention[data-type="#{type}"])).filter_map { mention_id(it)&.to_i }.uniq + end + def mention_anchor(mention) mention_instance = class_from_mention(mention) @@ -75,45 +112,61 @@ module OpenProject::TextFormatting end def work_package_mention(work_package, mention) - # Render the mention with the same label and URL convention used for - # `#N` text references elsewhere in the markdown pipeline. - display_id = work_package.display_id + return Nokogiri::XML::Text.new(work_package.formatted_id, mention.document) if text_only?(work_package) case mention.text.count("#") - when 3 - ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", - "", - data: { id: work_package.id, display_id:, detailed: true } - when 2 - ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", - "", - data: { id: work_package.id, display_id:, detailed: false } - else - link_to(work_package.formatted_id, - work_package_path_or_url(id: display_id, only_path: context[:only_path]), - class: "issue work_package", - data: { - hover_card_trigger_target: "trigger", - hover_card_url: hover_card_work_package_path(display_id) - }) + when 3 then work_package_quickinfo(work_package, detailed: true) + when 2 then work_package_quickinfo(work_package, detailed: false) + else work_package_link(work_package) end end - def class_from_mention(mention) - mention_class = case mention.attributes["data-type"].value - when "user" - User - when "group" - Group - when "work_package" - WorkPackage - else - raise ArgumentError - end + # The hover-card endpoint a quickinfo would link to is unreachable + # for plain-text recipients and for viewers without view permission. + def text_only?(work_package) + context[:plain_text] || @visible_mentioned_ids.exclude?(work_package.id) + end - mention_class - .visible - .find_by(id: mention_id(mention)) || fallback_text(mention) + def work_package_quickinfo(work_package, detailed:) + return work_package_static_macro(work_package, detailed:) if context[:static_html] + + ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", + "", + data: { id: work_package.id, + display_id: work_package.display_id, + detailed: } + end + + # Uses the WP's current `formatted_id` rather than the envelope text, + # so a renamed identifier doesn't leave a stale label in the mailer. + def work_package_static_macro(work_package, detailed:) + label = OpenProject::TextFormatting::Helpers::StaticMacroLabel + .call(work_package, label: work_package.formatted_id, detailed:) + + link_to(label, + work_package_path_or_url(id: work_package.display_id, only_path: context[:only_path]), + class: "issue work_package") + end + + def work_package_link(work_package) + display_id = work_package.display_id + link_to(work_package.formatted_id, + work_package_path_or_url(id: display_id, only_path: context[:only_path]), + class: "issue work_package", + data: { + hover_card_trigger_target: "trigger", + hover_card_url: hover_card_work_package_path(display_id) + }) + end + + def class_from_mention(mention) + id = mention_id(mention)&.to_i + case mention.attributes["data-type"].value + when "user" then @mentioned_users[id] + when "group" then @mentioned_groups[id] + when "work_package" then @mentioned_work_packages[id] + else raise ArgumentError + end || fallback_text(mention) end ## diff --git a/lib/open_project/text_formatting/filters/plain_text_output_filter.rb b/lib/open_project/text_formatting/filters/plain_text_output_filter.rb new file mode 100644 index 00000000000..34a2bbdada9 --- /dev/null +++ b/lib/open_project/text_formatting/filters/plain_text_output_filter.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module OpenProject::TextFormatting + module Filters + # Terminal stage when the rich pipeline is rendering for `text/plain` + # bodies — collapses the DOM to its visible text so no HTML escapes. + class PlainTextOutputFilter < HTML::Pipeline::Filter + def call + doc.text + end + end + end +end diff --git a/lib/open_project/text_formatting/formats/markdown/formatter.rb b/lib/open_project/text_formatting/formats/markdown/formatter.rb index 6c6cc3c1906..49696a41303 100644 --- a/lib/open_project/text_formatting/formats/markdown/formatter.rb +++ b/lib/open_project/text_formatting/formats/markdown/formatter.rb @@ -31,11 +31,45 @@ require "task_list/filter" module OpenProject::TextFormatting::Formats::Markdown class Formatter < OpenProject::TextFormatting::Formats::BaseFormatter + RICH_FILTERS = [ + OpenProject::TextFormatting::Filters::SettingMacrosFilter, + OpenProject::TextFormatting::Filters::MarkdownFilter, + OpenProject::TextFormatting::Filters::SanitizationFilter, + OpenProject::TextFormatting::Filters::TaskListFilter, + OpenProject::TextFormatting::Filters::TableOfContentsFilter, + OpenProject::TextFormatting::Filters::MacroFilter, + OpenProject::TextFormatting::Filters::MentionFilter, + OpenProject::TextFormatting::Filters::PatternMatcherFilter, + OpenProject::TextFormatting::Filters::SyntaxHighlightFilter, + OpenProject::TextFormatting::Filters::AttachmentFilter, + OpenProject::TextFormatting::Filters::AutolinkFilter, + OpenProject::TextFormatting::Filters::AutolinkCustomProtocolsFilter, + OpenProject::TextFormatting::Filters::RelativeLinkFilter, + OpenProject::TextFormatting::Filters::LinkAttributeFilter, + OpenProject::TextFormatting::Filters::ExternalLinkCaptureFilter, + OpenProject::TextFormatting::Filters::FigureWrappedFilter, + OpenProject::TextFormatting::Filters::BemCssFilter + ].freeze + + # `text/plain` mailer bodies share the matcher and mention stages so + # work-package references resolve consistently with the HTML channel, + # then `PlainTextOutputFilter` collapses the DOM to text. Filters that + # only shape HTML (TOC, syntax highlight, autolink, link-attribute, + # figure, BEM) are omitted because `doc.text` would discard their work. + TEXT_FILTERS = [ + OpenProject::TextFormatting::Filters::SettingMacrosFilter, + OpenProject::TextFormatting::Filters::MarkdownFilter, + OpenProject::TextFormatting::Filters::SanitizationFilter, + OpenProject::TextFormatting::Filters::MentionFilter, + OpenProject::TextFormatting::Filters::PatternMatcherFilter, + OpenProject::TextFormatting::Filters::PlainTextOutputFilter + ].freeze + def to_html(text) result = pipeline.call(text, context) output = result[:output].to_s - output.html_safe + context[:plain_text] ? output : output.html_safe # rubocop:disable Rails/OutputSafety end def to_document(text) @@ -43,25 +77,7 @@ module OpenProject::TextFormatting::Formats::Markdown end def filters - [ - OpenProject::TextFormatting::Filters::SettingMacrosFilter, - OpenProject::TextFormatting::Filters::MarkdownFilter, - OpenProject::TextFormatting::Filters::SanitizationFilter, - OpenProject::TextFormatting::Filters::TaskListFilter, - OpenProject::TextFormatting::Filters::TableOfContentsFilter, - OpenProject::TextFormatting::Filters::MacroFilter, - OpenProject::TextFormatting::Filters::MentionFilter, - OpenProject::TextFormatting::Filters::PatternMatcherFilter, - OpenProject::TextFormatting::Filters::SyntaxHighlightFilter, - OpenProject::TextFormatting::Filters::AttachmentFilter, - OpenProject::TextFormatting::Filters::AutolinkFilter, - OpenProject::TextFormatting::Filters::AutolinkCustomProtocolsFilter, - OpenProject::TextFormatting::Filters::RelativeLinkFilter, - OpenProject::TextFormatting::Filters::LinkAttributeFilter, - OpenProject::TextFormatting::Filters::ExternalLinkCaptureFilter, - OpenProject::TextFormatting::Filters::FigureWrappedFilter, - OpenProject::TextFormatting::Filters::BemCssFilter - ] + context[:plain_text] ? TEXT_FILTERS : RICH_FILTERS end def self.format diff --git a/lib/open_project/text_formatting/helpers/static_macro_label.rb b/lib/open_project/text_formatting/helpers/static_macro_label.rb new file mode 100644 index 00000000000..2c33e269044 --- /dev/null +++ b/lib/open_project/text_formatting/helpers/static_macro_label.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module OpenProject::TextFormatting + module Helpers + # Anchor text for the static-HTML form of a WP quickinfo macro: + # `[status ]type label: subject`. Used in channels (HTML mailers, + # server-side previews) that can't hydrate the `` widget. + module StaticMacroLabel + def self.call(work_package, label:, detailed:) + parts = [] + parts << work_package.status&.name if detailed + parts << work_package.type&.name + parts << label + "#{parts.compact.join(' ')}: #{work_package.subject}" + end + end + end +end diff --git a/lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb b/lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb index 4a3f85a492e..1f44e340e5f 100644 --- a/lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb +++ b/lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb @@ -78,41 +78,56 @@ module OpenProject::TextFormatting::Matchers # Both quickinfo and plain link need the WP record so the rendered # HTML can carry the record id in `data-id`. Unresolved WP → # literal text rather than a broken reference. - wp = OpenProject::TextFormatting::Matchers::ResourceLinksMatcher.work_package_for(display_id) + wp = preload_cache.fetch(display_id) return nil unless wp if quickinfo? - render_work_package_macro(id: wp.id, display_id: wp.display_id, detailed: detailed?) + render_work_package_macro(work_package: wp, fallback_id: display_id, detailed: detailed?) else render_work_package_link(wp, fallback_id: display_id) end end def render_for_numeric(wp_id) - wp = OpenProject::TextFormatting::Matchers::ResourceLinksMatcher.work_package_for(wp_id) + wp = preload_cache.fetch(wp_id) if quickinfo? - # Prefer the resolved WP's identifiers; fall back to the matched - # id when no preload is available (classic mode). - record_id = wp&.id || wp_id - display_id = wp&.display_id || wp_id - render_work_package_macro(id: record_id, display_id:, detailed: detailed?) + render_work_package_macro(work_package: wp, fallback_id: wp_id, detailed: detailed?) else render_work_package_link(wp, fallback_id: wp_id) end end - def render_work_package_macro(id:, display_id:, detailed: false) + def render_work_package_macro(work_package:, fallback_id:, detailed: false) + id = work_package&.id || fallback_id + display_id = work_package&.display_id || fallback_id + label = WorkPackage::SemanticIdentifier.format_display_id(display_id) + + return label if text_only?(work_package) + return render_static_work_package_macro(work_package, label, detailed:) if context[:static_html] + ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", data: { id:, display_id:, detailed: } end + # The label keeps what the author wrote (possibly a historical + # alias) so the rendered text matches the source markdown. + def render_static_work_package_macro(work_package, label, detailed:) + return label unless work_package + + link_to(OpenProject::TextFormatting::Helpers::StaticMacroLabel.call(work_package, label:, detailed:), + work_package_path_or_url(id: work_package.display_id, only_path: context[:only_path]), + class: "issue work_package") + end + def render_work_package_link(work_package, fallback_id:) # Fall back to the bare `#N` shape when no WP is provided (classic mode, # render path bypassing `PatternMatcherFilter`) rather than running a # per-link query inside the renderer. label = work_package&.formatted_id || "##{fallback_id}" + return label if text_only?(work_package) + href_id = work_package&.display_id || fallback_id link_to(label, @@ -123,6 +138,16 @@ module OpenProject::TextFormatting::Matchers hover_card_url: hover_card_work_package_path(href_id) }) end + + # A nil WP means classic mode skipped the preload, or the reference + # didn't resolve — neither case needs visibility gating. + def text_only?(work_package) + context[:plain_text] || (work_package && !preload_cache.visible?(work_package.id)) + end + + def preload_cache + OpenProject::TextFormatting::Matchers::ResourceLinksMatcher.current_cache + end end end end diff --git a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb index c49bfa3056c..667fb9afe19 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -69,8 +69,31 @@ module OpenProject::TextFormatting # identifier:version:1.0.0 # identifier:source:some/file class ResourceLinksMatcher < RegexMatcher - WORK_PACKAGES_LOOKUP_KEY = :text_formatting_work_packages_lookup - private_constant :WORK_PACKAGES_LOOKUP_KEY + # Unscoped label resolution (`lookup`) paired with viewer-scoped + # link gating (`visible_ids`), so the link handler renders the + # same label for everyone and decides anchor-vs-text per viewer. + class WorkPackagePreloadCache + attr_reader :lookup, :visible_ids + + def initialize(lookup:, visible_ids:) + @lookup = lookup + @visible_ids = visible_ids + end + + def fetch(identifier) + lookup[identifier.to_s] + end + + def visible?(work_package_id) + visible_ids.include?(work_package_id) + end + + # Frozen singleton, not a factory — callers must not mutate it. + EMPTY = new(lookup: {}.freeze, visible_ids: Set.new.freeze).freeze + end + + CACHE_KEY = :text_formatting_work_package_preload_cache + private_constant :CACHE_KEY include ::OpenProject::TextFormatting::Truncation # used for the work package quick links @@ -130,33 +153,33 @@ module OpenProject::TextFormatting ] end - # Returns the preloaded WorkPackage for the given identifier (numeric - # or semantic), or nil if no preload is active (classic mode, no `#N` - # references) or the WP couldn't be resolved. Lookup keys are always - # strings — see `index_by_id_and_identifier`. - def self.work_package_for(identifier) - RequestStore.store.dig(WORK_PACKAGES_LOOKUP_KEY, identifier.to_s) + def self.current_cache + RequestStore.store[CACHE_KEY] || WorkPackagePreloadCache::EMPTY end - # Doc-level preload called by `PatternMatcherFilter`. Save/restores - # the lookup so a nested `format_text` (e.g. custom-field formatter - # re-entering the pipeline) doesn't clobber the outer render. Classic - # mode skips the load — `display_id` collapses to numeric, so the - # link handler can render from the matched id alone. - def self.with_preloaded_resources(doc, _context) - previous = RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] - - return yield unless Setting::WorkPackageIdentifier.semantic? + # Save/restore so a nested `format_text` (e.g. a custom-field + # formatter re-entering the pipeline) doesn't clobber the outer + # render's cache. + def self.with_preloaded_resources(doc, context) + previous = RequestStore.store[CACHE_KEY] + return yield unless preload_required?(context) identifiers = collect_work_package_identifiers(doc) return yield if identifiers.empty? - RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = build_lookup(identifiers) + RequestStore.store[CACHE_KEY] = build_cache(identifiers, context) yield ensure - RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = previous + RequestStore.store[CACHE_KEY] = previous end + # Semantic mode needs the row to map `PROJ-7` to an id; static-HTML + # output needs `type`/`subject` to compose the quickinfo anchor. + def self.preload_required?(context) + Setting::WorkPackageIdentifier.semantic? || context[:static_html] + end + private_class_method :preload_required? + def self.collect_work_package_identifiers(doc) identifiers = Set.new doc.search(".//text()").each do |node| @@ -182,19 +205,25 @@ module OpenProject::TextFormatting identifier end - # 1 SELECT in the common case. A second targeted SELECT fires for - # historical aliases — the loaded WP row carries only its current - # identifier, so unmapped inputs must be filled in from - # `WorkPackageSemanticAlias`. The visible-id set passes through - # to the alias fold-in explicitly so each query enforces - # visibility at its own boundary, leaving no implicit trust for - # the cache to leak through. - def self.build_lookup(identifiers) - work_packages = WorkPackage.visible.where_display_id_in(*identifiers).select(:id, :identifier).to_a + # Two SELECTs in the common case (unscoped fetch + visibility id + # pluck), a third when historical aliases need resolving. Static- + # HTML output additionally needs `:type` and `:status` to compose + # the anchor for `##`/`###` macros. + def self.build_cache(identifiers, context = {}) + scope = WorkPackage.where_display_id_in(*identifiers) + scope = if context[:static_html] + scope.includes(:type, :status) + else + scope.select(:id, :identifier) + end + work_packages = scope.to_a + all_wp_ids = work_packages.map(&:id) + visible_ids = WorkPackage.visible.where(id: all_wp_ids).pluck(:id).to_set lookup = index_by_id_and_identifier(work_packages) - fold_in_alias_keys(lookup, identifiers, visible_wp_ids: work_packages.map(&:id)) - lookup + fold_in_alias_keys(lookup, identifiers, all_wp_ids:) + WorkPackagePreloadCache.new(lookup:, visible_ids:) end + private_class_method :build_cache # Keys are stringified at write time so callers can read with a single # `identifier.to_s` regardless of whether the input is a numeric id or @@ -207,12 +236,12 @@ module OpenProject::TextFormatting end private_class_method :index_by_id_and_identifier - def self.fold_in_alias_keys(lookup, identifiers, visible_wp_ids:) + def self.fold_in_alias_keys(lookup, identifiers, all_wp_ids:) unmapped = identifiers.map(&:to_s) - lookup.keys - return if unmapped.empty? || visible_wp_ids.empty? + return if unmapped.empty? || all_wp_ids.empty? WorkPackageSemanticAlias - .where(work_package_id: visible_wp_ids, identifier: unmapped) + .where(work_package_id: all_wp_ids, identifier: unmapped) .pluck(:identifier, :work_package_id) .each { |ident, wp_id| lookup[ident] = lookup[wp_id.to_s] } end diff --git a/lib/open_project/text_formatting/render_mode.rb b/lib/open_project/text_formatting/render_mode.rb new file mode 100644 index 00000000000..fd78397b739 --- /dev/null +++ b/lib/open_project/text_formatting/render_mode.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module OpenProject + module TextFormatting + # Maps a high-level rendering channel (`:in_app_html`, `:external_html`, + # `:external_text`) onto the primitive `only_path` / `static_html` / + # `plain_text` context flags that the filter pipeline reads. + # + # External surfaces always need absolute URLs *and* static rendering for + # JS-dependent components — the two flags are a coupled set. A single + # mode value is the canonical API; the primitives stay available as + # per-flag escape hatches for callers that need an asymmetric mix. + module RenderMode + DEFAULTS = { + in_app_html: { only_path: true, static_html: false, plain_text: false }.freeze, + external_html: { only_path: false, static_html: true, plain_text: false }.freeze, + external_text: { only_path: false, static_html: false, plain_text: true }.freeze + }.freeze + + module_function + + def resolve(mode, only_path: nil, static_html: nil, plain_text: nil) + defaults = DEFAULTS.fetch(mode) do + raise ArgumentError, "Unknown render_mode: #{mode.inspect}. " \ + "Expected one of #{DEFAULTS.keys.inspect}." + end + + { + only_path: only_path.nil? ? defaults[:only_path] : only_path, + static_html: static_html.nil? ? defaults[:static_html] : static_html, + plain_text: plain_text.nil? ? defaults[:plain_text] : plain_text + } + end + end + end +end diff --git a/spec/helpers/mail_formatting_helper_spec.rb b/spec/helpers/mail_formatting_helper_spec.rb new file mode 100644 index 00000000000..73c689b50d5 --- /dev/null +++ b/spec/helpers/mail_formatting_helper_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe MailFormattingHelper do + shared_let(:project) { create(:project, identifier: "macroproj") } + shared_let(:work_package) { create(:work_package, project:, subject: "test task") } + shared_let(:admin) { create(:admin) } + + before { login_as(admin) } + + describe "#format_mail_html" do + subject(:rendered) { helper.format_mail_html("see ###{work_package.id}") } + + it "renders the quickinfo macro as a static anchor (not the Angular custom element)" do + expect(rendered).to include(%(class="issue work_package)) + expect(rendered).not_to include("]*>\s*#{Regexp.escape(wp.formatted_id)}\s*}) + expect(rendered).not_to include(%(href="/work_packages/#{wp.display_id}")) + expect(rendered).not_to include("opce-macro-wp-quickinfo") + end + + it "renders a quickinfo envelope (`##`) as plain text too" do + wp = hidden_wp.reload + rendered = format_text(mention_tag(wp, sep: "##")) + + expect(rendered).to include(wp.formatted_id) + expect(rendered).not_to include("opce-macro-wp-quickinfo") + end + end + + context "in plain-text rendering mode", + with_settings: { work_packages_identifier: "semantic" } do + # `plain_text: true` must collapse mention envelopes to their current + # `formatted_id` so the `text/plain` mailer doesn't leak `` + # HTML or stale envelope text. + let(:project) { create(:project, identifier: "MACROPROJ") } + let(:work_package) { create(:work_package, project:, author:) } + + before { work_package.allocate_and_register_semantic_id } + + it "renders the formatted_id without an anchor or quickinfo" do + wp = work_package.reload + rendered = format_text(mention_tag(wp), plain_text: true) + + expect(rendered).to include(wp.formatted_id) + expect(rendered).not_to include("@#{user.name}) + end + + it "loads many mentioned users with a single users SELECT keyed by id" do + users = create_list(:user, 5, member_with_roles: { project => role }) + tags = users.map { |u| user_mention_tag(u) }.join + + recorder = ActiveRecord::QueryRecorder.new { format_text(tags) } + # Match SELECTs whose primary FROM is users (the column projection + # starts with `"users"."..."`), so permission subqueries with a + # nested `FROM "users"` don't get counted. + batched = recorder.log.grep(/\ASELECT "users"\.[^,]+,.*FROM "users"/i) + + expect(batched.size).to eq(1), + "expected exactly one batched users SELECT, got #{batched.size}:\n#{batched.join("\n")}" + end + end end end diff --git a/spec/lib/open_project/text_formatting/formats/markdown/static_html_rendering_spec.rb b/spec/lib/open_project/text_formatting/formats/markdown/static_html_rendering_spec.rb new file mode 100644 index 00000000000..f17f3ce4407 --- /dev/null +++ b/spec/lib/open_project/text_formatting/formats/markdown/static_html_rendering_spec.rb @@ -0,0 +1,224 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe "Markdown static-HTML rendering" do # rubocop:disable RSpec/DescribeClass + subject(:formatted) { render(input) } + + def render(text) + OpenProject::TextFormatting::Renderer.format_text( + text, + format: :rich, + **context.merge(static_html: true) + ) + end + + let(:context) { { only_path: false } } + + shared_let(:project) { create(:project, identifier: "demo") } + shared_let(:type) { create(:type, name: "Task") } + shared_let(:status) { create(:status, name: "New") } + shared_let(:work_package) do + create(:work_package, project:, type:, status:, subject: "Cats V Dogs") + end + shared_let(:admin) { create(:admin) } + + before { login_as(admin) } + + describe "no JS-hydrated custom elements" do + let(:input) { "see #{'##'}#{work_package.id} for details" } + + it "never emits " do + expect(formatted).not_to include("opce-macro-wp-quickinfo") + end + end + + describe "basic mention (#N) — no behaviour change" do + let(:input) { "see ##{work_package.id} please" } + + context "in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before { work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) } + + it "renders the formatted_id as an anchor" do + expect(formatted).to include(">DEMO-1<") + expect(formatted).to include('href="') + end + end + end + + describe "quickinfo macro (##N) — type + id + subject in static anchor" do + let(:input) { "see #{'##'}#{work_package.id}" } + + context "in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before { work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) } + + it "renders type + formatted_id + subject as a single anchor" do + expect(formatted).to match(%r{]*>Task DEMO-1: Cats V Dogs}) + end + + it "links to the work package show path" do + expect(formatted).to include(%(href="http)) + expect(formatted).to include("/work_packages/DEMO-1") + end + end + + context "in classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "renders type + #N + subject as a single anchor" do + expect(formatted).to match(%r{]*>Task ##{work_package.id}: Cats V Dogs}) + end + end + end + + describe "detailed macro (###N) — status + type + id + subject" do + let(:input) { "see #{'###'}#{work_package.id}" } + + context "in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before { work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) } + + it "renders status + type + formatted_id + subject as a single anchor" do + expect(formatted).to match(%r{]*>New Task DEMO-1: Cats V Dogs}) + end + end + + context "in classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "renders status + type + #N + subject as a single anchor" do + expect(formatted).to match(%r{]*>New Task ##{work_package.id}: Cats V Dogs}) + end + end + end + + describe "inaccessible work package" do + shared_let(:other_project) { create(:project, identifier: "secret") } + shared_let(:reader_role) { create(:project_role, permissions: %i[view_work_packages]) } + shared_let(:reader) { create(:user, member_with_roles: { project => reader_role }) } + shared_let(:hidden_wp) do + create(:work_package, project: other_project, type:, status:, subject: "Hidden") + end + + before { login_as(reader) } + + context "in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before { hidden_wp.update_columns(identifier: "SECRET-1", sequence_number: 1) } + + it "renders the bare identifier label without an anchor for ##N" do + rendered = render("see #{'##'}#{hidden_wp.id}") + expect(rendered).to include("SECRET-1") + expect(rendered).not_to match(%r{]*>[^<]*SECRET-1}) + expect(rendered).not_to include("Hidden") + end + + it "renders the bare identifier label without an anchor for ###N" do + rendered = render("see #{'###'}#{hidden_wp.id}") + expect(rendered).to include("SECRET-1") + expect(rendered).not_to match(%r{]*>[^<]*SECRET-1}) + end + end + + context "in classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "renders the bare #N label without an anchor for ##N" do + rendered = render("see #{'##'}#{hidden_wp.id}") + expect(rendered).to include("##{hidden_wp.id}") + expect(rendered).not_to match(%r{]*>[^<]*##{hidden_wp.id}}) + expect(rendered).not_to include("Hidden") + end + + it "renders the bare #N label without an anchor for ###N" do + rendered = render("see #{'###'}#{hidden_wp.id}") + expect(rendered).to include("##{hidden_wp.id}") + expect(rendered).not_to match(%r{]*>[^<]*##{hidden_wp.id}}) + end + end + end + + describe "work-package mention envelope" do + let(:mention_attrs) do + %(class="mention" data-id="#{work_package.id}" data-type="work_package" data-display-id="DEMO-1") + end + let(:input) { %(check ##DEMO-1) } + + context "in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before { work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) } + + it "renders ## quickinfo envelopes as a static anchor with type + id + subject" do + expect(formatted).to match(%r{]*>Task DEMO-1: Cats V Dogs}) + expect(formatted).not_to include("opce-macro-wp-quickinfo") + end + end + end + + # Public document exports and other non-authenticated rendering paths + # invoke the static-HTML pipeline with `User.current == User.anonymous`. + # In that context every non-public WP is invisible, so any mention must + # collapse to its current `formatted_id` as plain text — no anchor, no + # subject leak. + describe "anonymous current_user" do + shared_let(:private_project) { create(:project, identifier: "private", public: false) } + shared_let(:private_wp) do + create(:work_package, project: private_project, type:, status:, subject: "Top Secret") + end + + around do |example| + User.execute_as(User.anonymous) { example.run } + end + + context "in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before { private_wp.update_columns(identifier: "PRIVATE-1", sequence_number: 1) } + + it "does not raise and renders the identifier text" do + expect { render("see #{'##'}#{private_wp.id}") } + .not_to raise_error + + rendered = render("see #{'##'}#{private_wp.id}") + expect(rendered).to include("PRIVATE-1") + end + end + + context "in classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "does not raise and renders the #N text" do + expect { render("see #{'##'}#{private_wp.id}") } + .not_to raise_error + + rendered = render("see #{'##'}#{private_wp.id}") + expect(rendered).to include("##{private_wp.id}") + end + end + end +end diff --git a/spec/lib/open_project/text_formatting/formats/markdown/text_rendering_spec.rb b/spec/lib/open_project/text_formatting/formats/markdown/text_rendering_spec.rb new file mode 100644 index 00000000000..66be86e5ac3 --- /dev/null +++ b/spec/lib/open_project/text_formatting/formats/markdown/text_rendering_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe "Markdown plain-text rendering" do # rubocop:disable RSpec/DescribeClass + subject(:formatted) { render(input).strip } + + def render(text) + OpenProject::TextFormatting::Renderer.format_text(text, plain_text: true) + end + + describe "plain markdown" do + let(:input) { "Hello *world*" } + + it "renders text without HTML tags" do + expect(formatted).to eq("Hello world") + end + end + + describe "with an inline work-package reference" do + shared_let(:project) { create(:project, identifier: "demo") } + shared_let(:work_package) { create(:work_package, project:, subject: "task") } + shared_let(:admin) { create(:admin) } + + let(:input) { "see ##{work_package.id} please" } + + before { login_as(admin) } + + context "in classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "renders the hash-prefixed numeric id" do + expect(formatted).to eq("see ##{work_package.id} please") + end + end + + context "in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before do + work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) + end + + it "renders the bare semantic identifier" do + expect(formatted).to eq("see DEMO-1 please") + end + end + end + + describe "with a quickinfo macro reference" do + shared_let(:project) { create(:project, identifier: "demo") } + shared_let(:work_package) { create(:work_package, project:, subject: "task") } + shared_let(:admin) { create(:admin) } + + before { login_as(admin) } + + context "in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before do + work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) + end + + it "renders ##N as bare semantic identifier" do + expect(render("see #{'##'}#{work_package.id} please").strip).to eq("see DEMO-1 please") + end + + it "renders ###N as bare semantic identifier" do + expect(render("see #{'###'}#{work_package.id} please").strip).to eq("see DEMO-1 please") + end + end + + context "in classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "renders ##N as the hash-prefixed numeric id" do + expect(render("see #{'##'}#{work_package.id} please").strip).to eq("see ##{work_package.id} please") + end + + it "renders ###N as the hash-prefixed numeric id" do + expect(render("see #{'###'}#{work_package.id} please").strip).to eq("see ##{work_package.id} please") + end + end + end + + describe "with a work-package mention envelope" do + shared_let(:project) { create(:project, identifier: "demo") } + shared_let(:work_package) { create(:work_package, project:, subject: "task") } + shared_let(:admin) { create(:admin) } + + let(:mention_attrs) do + %(class="mention" data-id="#{work_package.id}" data-type="work_package" data-display-id="DEMO-1") + end + let(:input) { %(check #DEMO-1) } + + before { login_as(admin) } + + context "in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before do + work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) + end + + it "unwraps to the bare semantic identifier" do + expect(formatted).to eq("check DEMO-1") + end + end + + context "in classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "unwraps to the hash-prefixed numeric id" do + expect(formatted).to eq("check ##{work_package.id}") + end + end + + context "with a ##-shaped mention text in semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + let(:input) { %(check ##DEMO-1) } + + before do + work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) + end + + it "unwraps to the bare semantic identifier" do + expect(formatted).to eq("check DEMO-1") + end + end + end +end diff --git a/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb b/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb index ed0b41e2333..a819e110a82 100644 --- a/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb +++ b/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb @@ -94,17 +94,17 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages inner_doc = Nokogiri::HTML.fragment("##{inner.id}") matcher.with_preloaded_resources(outer_doc, {}) do - expect(matcher.work_package_for(outer.id)).to eq(outer) + expect(matcher.current_cache.fetch(outer.id)).to eq(outer) matcher.with_preloaded_resources(inner_doc, {}) do - expect(matcher.work_package_for(inner.id)).to eq(inner) + expect(matcher.current_cache.fetch(inner.id)).to eq(inner) end - expect(matcher.work_package_for(outer.id)) + expect(matcher.current_cache.fetch(outer.id)) .to eq(outer), "outer lookup should be restored after nested call" end - expect(matcher.work_package_for(outer.id)).to be_nil + expect(matcher.current_cache.fetch(outer.id)).to be_nil end end @@ -134,15 +134,17 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages include_context "with author signed in" let(:project) { create(:project, identifier: "NPLUSONE") } - it "loads referenced work packages with a single SELECT regardless of count" do + it "loads referenced work packages with a fixed two-SELECT preload regardless of count" do wps = create_list(:work_package, 5, project:, author:) ids_text = wps.map { |wp| "##{wp.id}" }.join(" ") recorder = ActiveRecord::QueryRecorder.new { format_text(ids_text) } wp_selects = recorder.log.grep(/FROM "work_packages"/i) - expect(wp_selects.size).to eq(1), - "expected exactly one work_packages SELECT, got #{wp_selects.size}:\n#{wp_selects.join("\n")}" + # One unscoped fetch by identifier (label resolution) plus one + # visibility-scoped pluck on the resulting ids (link gating). + expect(wp_selects.size).to eq(2), + "expected exactly two work_packages SELECTs, got #{wp_selects.size}:\n#{wp_selects.join("\n")}" end end @@ -199,7 +201,7 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages end context "with mixed numeric and semantic references in one render" do - it "resolves both with a single work_packages SELECT" do + it "resolves both with the fixed two-SELECT preload" do wps = create_list(:work_package, 2, project:, author:) wps.each(&:allocate_and_register_semantic_id) loaded = wps.map(&:reload) @@ -209,8 +211,8 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages recorder = ActiveRecord::QueryRecorder.new { rendered = format_text(text) } wp_selects = recorder.log.grep(/FROM "work_packages"/i) - expect(wp_selects.size).to eq(1), - "expected exactly one work_packages SELECT, got #{wp_selects.size}:\n#{wp_selects.join("\n")}" + expect(wp_selects.size).to eq(2), + "expected exactly two work_packages SELECTs, got #{wp_selects.size}:\n#{wp_selects.join("\n")}" # Both render with the user-facing display_id, regardless of which # form the user typed. @@ -220,7 +222,7 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages end context "with a historical alias reference" do - it "resolves via the alias table with two round-trips total" do + it "resolves via the alias table with bounded round-trips" do wp = work_package.reload # Simulate a project rename: the WP keeps its current MACROPROJ-N # identifier on the row, but a historical OLD-prefix alias row @@ -231,17 +233,17 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages rendered = nil recorder = ActiveRecord::QueryRecorder.new { rendered = format_text("see #OLDPROJ-1") } - # Two targeted round-trips: (1) `where_display_id_in` runs a - # single WP SELECT whose WHERE clause includes an EXISTS - # subquery against the alias table; (2) a sidecar alias pluck - # maps the historical input string back to its WP for the - # cache. Scoped greps ignore incidental Setting/permission - # queries — a second match on either grep would indicate an + # Bounded round-trips: (1) `where_display_id_in` runs an unscoped + # WP SELECT whose WHERE includes an EXISTS subquery against the + # alias table, (2) a visibility-scoped id pluck for link gating, + # (3) a sidecar alias pluck maps the historical input string back + # to its WP for the cache. Scoped greps ignore incidental + # Setting/permission queries — additional matches indicate an # N+1 regression. wp_selects = recorder.log.grep(/FROM "work_packages"/) alias_selects = recorder.log.grep(/FROM "work_package_semantic_aliases"/) .grep_v(/FROM "work_packages"/) - expect(wp_selects.size).to eq(1) + expect(wp_selects.size).to eq(2) expect(alias_selects.size).to eq(1) # Renders against the WP's CURRENT display_id, not the historical @@ -298,11 +300,11 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages describe "visibility scoping", with_settings: { work_packages_identifier: "semantic" } do - # The lookup cache must scope through `WorkPackage.visible` — - # anything it surfaces ends up in the rendered link, so an - # unscoped cache would let any user read back the project - # identifier of a WP just by guessing its primary key, semantic - # identifier, or historical alias. + # Label resolution is unscoped so notification recipients see the same + # identifier shape as authors, but anchors are still gated by + # `WorkPackage.visible` — the link handler emits a plain-text label + # for inaccessible WPs rather than a navigable URL or hover-card + # endpoint. include_context "with author signed in" let(:project) { create(:project, identifier: "VISIBLE") } @@ -316,46 +318,44 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages end context "with a semantic-shaped ref to an inaccessible work package" do - it "renders literal text and never surfaces the WP's display id" do + it "renders the formatted_id as plain text with no anchor or quickinfo" do wp = hidden_wp.reload rendered = format_text("see ##{wp.display_id} here") - expect(rendered).to include("##{wp.display_id}") + expect(rendered).to include(wp.formatted_id) + expect(rendered).not_to match(%r{]*>\s*#{Regexp.escape(wp.formatted_id)}\s*}) expect(rendered).not_to include(%(href="/work_packages/#{wp.display_id}")) expect(rendered).not_to include("opce-macro-wp-quickinfo") end end context "with a numeric ref to an inaccessible work package in semantic mode" do - it "renders the numeric label and href without upgrading to the semantic identifier" do + it "upgrades the label to the formatted_id but does not render an anchor" do wp = hidden_wp.reload rendered = format_text("see ##{wp.id} here") - # The link still renders — `#42` was already in the user's input — - # but the upgrade to the WP's `formatted_id` / `display_id` (which - # would leak the project identifier) does not happen. - expect(rendered).to include(%(href="/work_packages/#{wp.id}")) - expect(rendered).to include(">##{wp.id}<") + expect(rendered).to include(wp.formatted_id) + expect(rendered).not_to match(%r{]*>\s*#{Regexp.escape(wp.formatted_id)}\s*}) + expect(rendered).not_to include(%(href="/work_packages/#{wp.id}")) expect(rendered).not_to include(%(href="/work_packages/#{wp.display_id}")) - expect(rendered).not_to include(">#{wp.formatted_id}<") end end context "with a historical alias for an inaccessible work package" do - it "renders literal text and does not resolve via the alias table" do + it "resolves the alias and renders the current formatted_id as plain text" do wp = hidden_wp.reload WorkPackageSemanticAlias.create!(work_package_id: wp.id, identifier: "OLDHIDDEN-1") rendered = format_text("see #OLDHIDDEN-1 here") - expect(rendered).to include("#OLDHIDDEN-1") + expect(rendered).to include(wp.formatted_id) + expect(rendered).not_to match(%r{]*>\s*#{Regexp.escape(wp.formatted_id)}\s*}) expect(rendered).not_to include(%(href="/work_packages/#{wp.display_id}")) - expect(rendered).not_to include(">#{wp.formatted_id}<") end end context "with visible and invisible refs mixed in one input" do - it "renders the visible ref normally and falls back to literal text for the invisible one" do + it "renders the visible ref as an anchor and the invisible ref as plain-text label" do visible = visible_wp.reload hidden = hidden_wp.reload rendered = format_text("see ##{visible.display_id} and ##{hidden.display_id}") @@ -364,7 +364,8 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages expect(rendered).to include(">#{visible.formatted_id}<") expect(rendered).not_to include(%(href="/work_packages/#{hidden.display_id}")) - expect(rendered).to include("##{hidden.display_id}") + expect(rendered).to include(hidden.formatted_id) + expect(rendered).not_to match(%r{]*>\s*#{Regexp.escape(hidden.formatted_id)}\s*}) end end end diff --git a/spec/lib/open_project/text_formatting/render_mode_spec.rb b/spec/lib/open_project/text_formatting/render_mode_spec.rb new file mode 100644 index 00000000000..843e3fa989f --- /dev/null +++ b/spec/lib/open_project/text_formatting/render_mode_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe OpenProject::TextFormatting::RenderMode do + describe ".resolve" do + context "with :in_app_html" do + it "produces the in-app default trio" do + expect(described_class.resolve(:in_app_html)).to eq( + only_path: true, + static_html: false, + plain_text: false + ) + end + end + + context "with :external_html" do + it "produces absolute URLs and static-HTML rendering" do + expect(described_class.resolve(:external_html)).to eq( + only_path: false, + static_html: true, + plain_text: false + ) + end + end + + context "with :external_text" do + it "produces absolute URLs and plain-text output" do + expect(described_class.resolve(:external_text)).to eq( + only_path: false, + static_html: false, + plain_text: true + ) + end + end + + context "with an explicit primitive flag" do + it "lets only_path override while keeping the rest of the mode's defaults" do + expect(described_class.resolve(:external_html, only_path: true)).to eq( + only_path: true, + static_html: true, + plain_text: false + ) + end + + it "lets an explicit false override the mode's true default" do + expect(described_class.resolve(:external_html, static_html: false)).to eq( + only_path: false, + static_html: false, + plain_text: false + ) + end + + it "ignores a nil override (treats it as 'not passed')" do + expect(described_class.resolve(:external_html, plain_text: nil)).to eq( + only_path: false, + static_html: true, + plain_text: false + ) + end + end + + context "with an unknown mode" do + it "raises ArgumentError naming the bad value" do + expect { described_class.resolve(:nonsense) } + .to raise_error(ArgumentError, /render_mode.*nonsense/) + end + end + end +end diff --git a/spec/mailers/work_package_mailer_spec.rb b/spec/mailers/work_package_mailer_spec.rb index a2028ce1e74..93fab35f2f2 100644 --- a/spec/mailers/work_package_mailer_spec.rb +++ b/spec/mailers/work_package_mailer_spec.rb @@ -129,6 +129,42 @@ RSpec.describe WorkPackageMailer do expect(mail["X-OpenProject-WorkPackage-Assignee"].value) .to eql work_package.assigned_to.login end + + describe "rendering a journal note containing a WP reference" do + shared_let(:persisted_project) { create(:project, identifier: "demo") } + shared_let(:persisted_recipient) { create(:admin) } + shared_let(:referenced_wp) { create(:work_package, project: persisted_project, subject: "child") } + shared_let(:parent_wp) { create(:work_package, project: persisted_project, subject: "parent") } + + let(:persisted_journal) do + create(:work_package_journal, + journable: parent_wp, + user: persisted_recipient, + version: parent_wp.journals.maximum(:version).to_i + 1, + notes: "see ##{referenced_wp.id}") + end + let(:mail) { described_class.mentioned(persisted_recipient, persisted_journal) } + + context "with classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "renders the hash-prefixed numeric id in the text body" do + expect(mail.text_part.body.to_s).to include("##{referenced_wp.id}") + end + end + + context "with semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before do + referenced_wp.update_columns(identifier: "DEMO-1", sequence_number: 1) + end + + it "renders the bare semantic identifier in the text body" do + body = mail.text_part.body.to_s + expect(body).to include("DEMO-1") + expect(body).not_to match(/##{referenced_wp.id}\b/) + end + end + end end describe "#watcher_changed" do @@ -209,5 +245,135 @@ RSpec.describe WorkPackageMailer do .to eql "op.work_package-#{work_package.id}@example.net" end end + + describe "rendering the latest comment containing a WP reference" do + shared_let(:persisted_project) { create(:project, identifier: "demo") } + shared_let(:persisted_recipient) { create(:admin) } + shared_let(:referenced_wp) { create(:work_package, project: persisted_project, subject: "child") } + shared_let(:parent_wp) { create(:work_package, project: persisted_project, subject: "parent") } + + let(:mail) do + create(:work_package_journal, + journable: parent_wp, + user: persisted_recipient, + version: parent_wp.journals.maximum(:version).to_i + 1, + notes: "Updated automatically by changing values within child work package ##{referenced_wp.id}") + described_class.watcher_changed(parent_wp, persisted_recipient, persisted_recipient, "added") + end + + context "with classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "renders the hash-prefixed numeric id in the text body" do + expect(mail.text_part.body.to_s).to include("##{referenced_wp.id}") + end + end + + context "with semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before do + referenced_wp.update_columns(identifier: "DEMO-1", sequence_number: 1) + end + + it "renders the bare semantic identifier in the text body" do + body = mail.text_part.body.to_s + expect(body).to include("DEMO-1") + expect(body).not_to match(/##{referenced_wp.id}\b/) + end + + it "renders the bare semantic identifier in the html body" do + body = mail.html_part.body.to_s + expect(body).to include("DEMO-1") + end + end + end + + describe "rendering a cross-project WP reference to a recipient without visibility", + with_settings: { work_packages_identifier: "semantic" } do + shared_let(:parent_project) { create(:project, identifier: "parent-proj") } + shared_let(:child_project) { create(:project, identifier: "child-proj") } + shared_let(:parent_wp) { create(:work_package, project: parent_project, subject: "parent") } + shared_let(:child_wp) { create(:work_package, project: child_project, subject: "child") } + shared_let(:reader_role) { create(:project_role, permissions: %i[view_work_packages]) } + shared_let(:reader) { create(:user, member_with_roles: { parent_project => reader_role }) } + + let(:mail) do + child_wp.update_columns(identifier: "CHILDPROJ-1", sequence_number: 1) + create(:work_package_journal, + journable: parent_wp, + user: reader, + version: parent_wp.journals.maximum(:version).to_i + 1, + notes: "Updated automatically by changing values within child work package ##{child_wp.id}") + described_class.watcher_changed(parent_wp, reader, reader, "added") + end + + it "renders the semantic identifier as plain text in the text body" do + body = mail.text_part.body.to_s + expect(body).to include("CHILDPROJ-1") + expect(body).not_to match(/##{child_wp.id}\b/) + end + + it "renders the semantic identifier without an anchor in the html body" do + body = mail.html_part.body.to_s + expect(body).to include("CHILDPROJ-1") + expect(body).not_to include(%(href="/work_packages/#{child_wp.id}")) + expect(body).not_to include(%(href="/work_packages/CHILDPROJ-1")) + end + end + + describe "rendering a quickinfo/detailed macro in the latest comment" do + shared_let(:persisted_project) { create(:project, identifier: "demo") } + shared_let(:persisted_recipient) { create(:admin) } + shared_let(:macro_type) { create(:type, name: "Task") } + shared_let(:macro_status) { create(:status, name: "New") } + shared_let(:referenced_wp) do + create(:work_package, + project: persisted_project, + type: macro_type, + status: macro_status, + subject: "Cats V Dogs") + end + shared_let(:parent_wp) { create(:work_package, project: persisted_project, subject: "parent") } + + let(:mail) do + create(:work_package_journal, + journable: parent_wp, + user: persisted_recipient, + version: parent_wp.journals.maximum(:version).to_i + 1, + notes: "ref ##{referenced_wp.id} ##{'#'}#{referenced_wp.id} ###{'#'}#{referenced_wp.id}") + described_class.watcher_changed(parent_wp, persisted_recipient, persisted_recipient, "added") + end + + context "with semantic mode", + with_settings: { work_packages_identifier: "semantic" } do + before { referenced_wp.update_columns(identifier: "DEMO-1", sequence_number: 1) } + + it "renders ## quickinfo as a static anchor with type + id + subject" do + body = mail.html_part.body.to_s + expect(body).to match(%r{]*>Task DEMO-1: Cats V Dogs}) + end + + it "renders ### detailed as a static anchor with status + type + id + subject" do + body = mail.html_part.body.to_s + expect(body).to match(%r{]*>New Task DEMO-1: Cats V Dogs}) + end + + it "never leaks into the html body" do + expect(mail.html_part.body.to_s).not_to include("opce-macro-wp-quickinfo") + end + end + + context "with classic mode", + with_settings: { work_packages_identifier: "classic" } do + it "renders ## quickinfo as a static anchor with type + #N + subject" do + body = mail.html_part.body.to_s + expect(body).to match(%r{]*>Task ##{referenced_wp.id}: Cats V Dogs}) + end + + it "renders ### detailed as a static anchor with status + type + #N + subject" do + body = mail.html_part.body.to_s + expect(body).to match(%r{]*>New Task ##{referenced_wp.id}: Cats V Dogs}) + end + end + end end end diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 229cba5fa67..c07349d42c1 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -606,6 +606,20 @@ RSpec.describe WorkPackage::SemanticIdentifier do end end + describe ".format_display_id" do + it "returns the semantic identifier unchanged when it carries letters" do + expect(described_class.format_display_id("MYPROJ-1")).to eq("MYPROJ-1") + end + + it "hash-prefixes a numeric integer" do + expect(described_class.format_display_id(42)).to eq("#42") + end + + it "hash-prefixes a numeric string" do + expect(described_class.format_display_id("42")).to eq("#42") + end + end + describe "#to_param" do include Rails.application.routes.url_helpers diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index 73606ebb077..3a9ad8fc105 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -1385,4 +1385,45 @@ RSpec.describe WorkPackages::UpdateAncestorsService, end end end + + describe "auto-generated journal note when a child triggers an ancestor recompute", + with_settings: { work_package_done_ratio: "status" } do + shared_let_work_packages(<<~TABLE) + hierarchy | status | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | Open | 10h | 15h | 10h | 15h | 0% | 0% + child | Open | 5h | | 5h | | 0% | + TABLE + + # The journal note always stores the primary-key reference (`#42`). + # Render-time resolution in the formatter pipeline turns it into + # `#PROJ-7` in semantic mode and `#42` in classic mode, so the stored + # text survives project-identifier renames. + context "in classic mode", + with_settings: { work_package_done_ratio: "status", work_packages_identifier: "classic" } do + it "writes the child's hash-prefixed primary key into the parent's journal note" do + set_attributes_on(child, status: closed_status) + call_update_ancestors_service(child) + + note = parent.reload.journals.last.notes + expect(note).to include("##{child.id}") + end + end + + context "in semantic mode", + with_settings: { work_package_done_ratio: "status", work_packages_identifier: "semantic" } do + before do + child.allocate_and_register_semantic_id + end + + it "writes the child's hash-prefixed primary key, not its semantic identifier" do + set_attributes_on(child, status: closed_status) + call_update_ancestors_service(child) + + wp = child.reload + note = parent.reload.journals.last.notes + expect(note).to include("##{wp.id}") + expect(note).not_to include(wp.identifier) + end + end + end end