From 7c9d15e5065babad6908f2874d96e06e960cf9ba Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 25 May 2026 14:57:37 +0300 Subject: [PATCH 01/20] Render WP identifiers per current mode in plain-text mailer notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `mentioned` and `watcher_changed` text-mailer bodies surfaced raw journal markdown — numeric `#42` references stayed numeric in semantic mode, and `` envelopes leaked as HTML source. Introduces `:plain_text` as a sibling format inside the existing Plain module. The filter chain mirrors the markdown pipeline (markdown, sanitization, mention, pattern-matcher) and finishes with a new `PlainTextOutputFilter` that collapses the DOM to text. The `WorkPackages` link handler and `MentionFilter` get plain-text branches keyed off `context[:plain_text]` so identifier resolution stays in one place across rich and plain channels. Closes https://community.openproject.org/wp/74762 --- .../work_package/semantic_identifier.rb | 9 +- .../work_package_mailer/mentioned.text.erb | 8 +- .../watcher_changed.text.erb | 8 +- .../text_formatting/filters/mention_filter.rb | 44 +++--- .../filters/plain_text_output_filter.rb | 43 +++++ .../formats/plain/text_formatter.rb | 61 ++++++++ .../matchers/link_handlers/work_packages.rb | 4 + lib/open_project/text_formatting/renderer.rb | 4 +- .../formats/plain/text_formatter_spec.rb | 148 ++++++++++++++++++ spec/mailers/work_package_mailer_spec.rb | 81 ++++++++++ .../work_package/semantic_identifier_spec.rb | 14 ++ 11 files changed, 400 insertions(+), 24 deletions(-) create mode 100644 lib/open_project/text_formatting/filters/plain_text_output_filter.rb create mode 100644 lib/open_project/text_formatting/formats/plain/text_formatter.rb create mode 100644 spec/lib/open_project/text_formatting/formats/plain/text_formatter_spec.rb diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index b743f058b42..4dbd5dc76f2 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -134,8 +134,13 @@ 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) + end + + # Module-level variant for callers that already hold a display id and + # don't need the WorkPackage record. + def self.format(display_id) + display_id.is_a?(String) && display_id.match?(/[A-Za-z]/) ? display_id : "##{display_id}" end # Override ActiveRecord's default `to_param` so Rails URL helpers diff --git a/app/views/work_package_mailer/mentioned.text.erb b/app/views/work_package_mailer/mentioned.text.erb index eee14ad5c8b..45a2c8766e4 100644 --- a/app/views/work_package_mailer/mentioned.text.erb +++ b/app/views/work_package_mailer/mentioned.text.erb @@ -8,6 +8,12 @@ <%= "=" * ((@work_package.formatted_id + " " + @work_package.subject).length + 4) %> <%= I18n.t(:label_comment_added) %>: -<%= strip_tags @journal.notes %> +<%= format_text( + @journal.notes, + format: :plain_text, + only_path: false, + object: @work_package, + project: @work_package.project + ) %> <%= "-" * 100 %> diff --git a/app/views/work_package_mailer/watcher_changed.text.erb b/app/views/work_package_mailer/watcher_changed.text.erb index 7de3fc8bccc..6dceb846e5e 100644 --- a/app/views/work_package_mailer/watcher_changed.text.erb +++ b/app/views/work_package_mailer/watcher_changed.text.erb @@ -31,4 +31,10 @@ 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_text( + t(:text_latest_note, note: last_work_package_note(@work_package)), + format: :plain_text, + only_path: false, + object: @work_package, + project: @work_package.project + ) %> diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 4563289e07b..9c2740d63e8 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -75,30 +75,36 @@ 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 context[:plain_text] + # Match the label and URL convention used for `#N` text references + # elsewhere in the markdown pipeline. 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 work_package_quickinfo(work_package, detailed:) + ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", + "", + data: { id: work_package.id, + display_id: work_package.display_id, + detailed: } + 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) mention_class = case mention.attributes["data-type"].value when "user" 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..fb3b0511059 --- /dev/null +++ b/lib/open_project/text_formatting/filters/plain_text_output_filter.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. +#++ + +module OpenProject::TextFormatting + module Filters + # Final stage of the plain-text pipeline. Earlier filters resolve + # mentions and macros to their text-mode shapes (driven by + # `context[:plain_text]`); this stage collapses any remaining markup + # so the pipeline output is suitable for `text/plain` bodies. + class PlainTextOutputFilter < HTML::Pipeline::Filter + def call + doc.text + end + end + end +end diff --git a/lib/open_project/text_formatting/formats/plain/text_formatter.rb b/lib/open_project/text_formatting/formats/plain/text_formatter.rb new file mode 100644 index 00000000000..8dfb25ac19c --- /dev/null +++ b/lib/open_project/text_formatting/formats/plain/text_formatter.rb @@ -0,0 +1,61 @@ +# 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::Formats + module Plain + # Plain-text output sibling of `Plain::Formatter`. Shares the matcher + # and mention pipeline with the rich (markdown) renderer so identifier + # resolution stays consistent across channels, but collapses the final + # DOM to text. Intended for plain/text mailers and similar contexts + # where HTML would be a foreign body. + class TextFormatter < OpenProject::TextFormatting::Formats::BaseFormatter + def initialize(context) + super(context.merge(plain_text: true)) + end + + def to_html(text) + pipeline.call(text, context)[:output].to_s + end + + def 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 + ] + end + + def self.format = :plain_text + 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..4a18ec077c1 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 @@ -103,6 +103,8 @@ module OpenProject::TextFormatting::Matchers end def render_work_package_macro(id:, display_id:, detailed: false) + return WorkPackage::SemanticIdentifier.format(display_id) if context[:plain_text] + ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", data: { id:, display_id:, detailed: } @@ -113,6 +115,8 @@ module OpenProject::TextFormatting::Matchers # render path bypassing `PatternMatcherFilter`) rather than running a # per-link query inside the renderer. label = work_package&.formatted_id || "##{fallback_id}" + return label if context[:plain_text] + href_id = work_package&.display_id || fallback_id link_to(label, diff --git a/lib/open_project/text_formatting/renderer.rb b/lib/open_project/text_formatting/renderer.rb index fd77761737e..e251f0cd894 100644 --- a/lib/open_project/text_formatting/renderer.rb +++ b/lib/open_project/text_formatting/renderer.rb @@ -49,12 +49,14 @@ module OpenProject::TextFormatting .to_html(text) end - # @param [:plain, :rich] format the text format. + # @param [:plain, :plain_text, :rich] format the text format. # @return [Formats::BaseFormatter] a formatter implementation. def formatter_for(format) case format.to_sym when :plain Formats.plain_formatter + when :plain_text + Formats::Plain::TextFormatter else Formats.rich_formatter end diff --git a/spec/lib/open_project/text_formatting/formats/plain/text_formatter_spec.rb b/spec/lib/open_project/text_formatting/formats/plain/text_formatter_spec.rb new file mode 100644 index 00000000000..2e46cc2570c --- /dev/null +++ b/spec/lib/open_project/text_formatting/formats/plain/text_formatter_spec.rb @@ -0,0 +1,148 @@ +# 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::Formats::Plain::TextFormatter do + subject(:formatted) { described_class.new(context).to_html(input).strip } + + let(:context) { {} } + + 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_flag: { semantic_work_package_ids: false }, + 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_flag: { semantic_work_package_ids: true }, + 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_flag: { semantic_work_package_ids: true }, + 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 + input = "see #{'##'}#{work_package.id} please" + expect(described_class.new({}).to_html(input).strip).to eq("see DEMO-1 please") + end + + it "renders ###N as bare semantic identifier" do + input = "see #{'###'}#{work_package.id} please" + expect(described_class.new({}).to_html(input).strip).to eq("see DEMO-1 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_flag: { semantic_work_package_ids: true }, + 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_flag: { semantic_work_package_ids: false }, + 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_flag: { semantic_work_package_ids: true }, + 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/mailers/work_package_mailer_spec.rb b/spec/mailers/work_package_mailer_spec.rb index a2028ce1e74..254549fa377 100644 --- a/spec/mailers/work_package_mailer_spec.rb +++ b/spec/mailers/work_package_mailer_spec.rb @@ -129,6 +129,44 @@ 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_flag: { semantic_work_package_ids: false }, + 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_flag: { semantic_work_package_ids: true }, + 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 +247,48 @@ 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_flag: { semantic_work_package_ids: false }, + 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_flag: { semantic_work_package_ids: true }, + 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 end end diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 229cba5fa67..cbd8a35731e 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" do + it "returns the semantic identifier unchanged when it carries letters" do + expect(described_class.format("MYPROJ-1")).to eq("MYPROJ-1") + end + + it "hash-prefixes a numeric integer" do + expect(described_class.format(42)).to eq("#42") + end + + it "hash-prefixes a numeric string" do + expect(described_class.format("42")).to eq("#42") + end + end + describe "#to_param" do include Rails.application.routes.url_helpers From 878048f8e80af45434a52de6722749e83470e473 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 25 May 2026 15:40:40 +0300 Subject: [PATCH 02/20] Resolve WP labels across visibility boundaries in text macros MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macro preload was visibility-scoped — references to work packages the recipient cannot see would fall through to the literal `#43` shape, even when the same reference rendered as `DCP-1` for an author with full view permission. Notification recipients saw misleading numeric ids for cross- project references in journal notes. Splits label resolution from link gating: - `ResourceLinksMatcher.build_lookup` now does an unscoped fetch for the primary identifier and a separate visibility-scoped id pluck. The link handler reads `visible_to_current_user?` to decide between a navigable anchor and a plain-text label. - `UpdateAncestorsService#set_journal_note` writes `#display_id` so new notes carry the semantic shape at the source; render-time resolution heals legacy `#N` content for users with view permission. Tradeoff: a recipient without view permission now sees the WP's semantic identifier (e.g. `DCP-1`) as plain text rather than `#43`. The reference's existence was already disclosed by the stored journal text; the project identifier is the only new piece of information surfaced, and is not treated as a secret elsewhere in the system (URLs, exports, API). --- .../work_packages/update_ancestors_service.rb | 3 +- .../matchers/link_handlers/work_packages.rb | 23 ++++--- .../matchers/resource_links_matcher.rb | 67 ++++++++++++++----- .../link_handlers/work_packages_spec.rb | 67 ++++++++++--------- spec/mailers/work_package_mailer_spec.rb | 34 ++++++++++ .../update_ancestors_service_spec.rb | 39 +++++++++++ 6 files changed, 173 insertions(+), 60 deletions(-) diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index 7f4ac6c0d67..966766ee3b3 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.display_id}") 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 4a18ec077c1..755e67c1dd1 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 @@ -82,7 +82,7 @@ module OpenProject::TextFormatting::Matchers 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 @@ -92,18 +92,19 @@ module OpenProject::TextFormatting::Matchers wp = OpenProject::TextFormatting::Matchers::ResourceLinksMatcher.work_package_for(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) - return WorkPackage::SemanticIdentifier.format(display_id) if context[:plain_text] + 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) + + return label if context[:plain_text] + return label if work_package && !visible_to_current_user?(work_package.id) ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", @@ -116,6 +117,7 @@ module OpenProject::TextFormatting::Matchers # per-link query inside the renderer. label = work_package&.formatted_id || "##{fallback_id}" return label if context[:plain_text] + return label if work_package && !visible_to_current_user?(work_package.id) href_id = work_package&.display_id || fallback_id @@ -127,6 +129,11 @@ module OpenProject::TextFormatting::Matchers hover_card_url: hover_card_work_package_path(href_id) }) end + + def visible_to_current_user?(work_package_id) + OpenProject::TextFormatting::Matchers::ResourceLinksMatcher + .visible_to_current_user?(work_package_id) + 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..e558eb677b9 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -70,7 +70,8 @@ module OpenProject::TextFormatting # identifier:source:some/file class ResourceLinksMatcher < RegexMatcher WORK_PACKAGES_LOOKUP_KEY = :text_formatting_work_packages_lookup - private_constant :WORK_PACKAGES_LOOKUP_KEY + VISIBLE_WORK_PACKAGE_IDS_KEY = :text_formatting_visible_work_package_ids + private_constant :WORK_PACKAGES_LOOKUP_KEY, :VISIBLE_WORK_PACKAGE_IDS_KEY include ::OpenProject::TextFormatting::Truncation # used for the work package quick links @@ -138,25 +139,49 @@ module OpenProject::TextFormatting RequestStore.store.dig(WORK_PACKAGES_LOOKUP_KEY, identifier.to_s) end + # The main lookup is unscoped so labels resolve regardless of + # current-user permissions. This predicate gates whether the + # link handler emits a navigable anchor or a plain-text label. + def self.visible_to_current_user?(work_package_id) + RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY]&.include?(work_package_id) || false + 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] - + previous = stash_preload_state return yield unless Setting::WorkPackageIdentifier.semantic? identifiers = collect_work_package_identifiers(doc) return yield if identifiers.empty? - RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = build_lookup(identifiers) + install_preload_state(*build_lookup(identifiers)) yield ensure - RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = previous + restore_preload_state(previous) end + def self.stash_preload_state + [RequestStore.store[WORK_PACKAGES_LOOKUP_KEY], + RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY]] + end + private_class_method :stash_preload_state + + def self.install_preload_state(lookup, visible_ids) + RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = lookup + RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY] = visible_ids + end + private_class_method :install_preload_state + + def self.restore_preload_state(previous) + RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = previous[0] + RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY] = previous[1] + end + private_class_method :restore_preload_state + def self.collect_work_package_identifiers(doc) identifiers = Set.new doc.search(".//text()").each do |node| @@ -182,18 +207,24 @@ 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. + # Label resolution is unscoped: a reference to an inaccessible + # work package still renders as its semantic identifier (e.g. + # `DCP-1`), just without a navigable anchor. Visibility is + # captured separately so the link handler can pick anchor-vs-text + # per WP without re-querying. + # + # Two SELECTs in the common case: one unscoped fetch by identifier, + # one visibility-scoped id pluck. A third targeted SELECT fires + # for historical aliases — the loaded row carries only the current + # identifier, so unmapped inputs are filled in from + # `WorkPackageSemanticAlias`. def self.build_lookup(identifiers) - work_packages = WorkPackage.visible.where_display_id_in(*identifiers).select(:id, :identifier).to_a + work_packages = WorkPackage.where_display_id_in(*identifiers).select(:id, :identifier).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:) + [lookup, visible_ids] end # Keys are stringified at write time so callers can read with a single @@ -207,12 +238,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/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..14a2a1cbbc4 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 @@ -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/mailers/work_package_mailer_spec.rb b/spec/mailers/work_package_mailer_spec.rb index 254549fa377..64df0a68ab5 100644 --- a/spec/mailers/work_package_mailer_spec.rb +++ b/spec/mailers/work_package_mailer_spec.rb @@ -290,5 +290,39 @@ RSpec.describe WorkPackageMailer do end end end + + describe "rendering a cross-project WP reference to a recipient without visibility", + with_flag: { semantic_work_package_ids: true }, + 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 end end diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index 73606ebb077..a18e3c44d3f 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -1385,4 +1385,43 @@ 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 + + context "in classic mode", + with_flag: { semantic_work_package_ids: false }, + with_settings: { work_package_done_ratio: "status", work_packages_identifier: "classic" } do + it "writes the child's hash-prefixed numeric id 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_flag: { semantic_work_package_ids: true }, + 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 semantic identifier into the parent's journal note" 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.display_id}") + expect(note).not_to match(/##{wp.id}\b/) + end + end + end end From 8d9aa18ad3ff769a2da49130c2c4d8932852db92 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 25 May 2026 16:31:56 +0300 Subject: [PATCH 03/20] Collapse work-package preload state into one cache value object Pairs unscoped label resolution and viewer-scoped link gating in a WorkPackagePreloadCache instead of two RequestStore keys with a five-method save/restore protocol. Exposes one `current_cache` reader; consumers ask the cache directly via `fetch` and `visible?`. Extracts a `text_only?` predicate in the WP link handler so the `context[:plain_text]` and invisible-WP guards collapse into a single call site. `SemanticIdentifier.format` renames its parameter to reflect that the input may or may not be semantic. --- .../work_package/semantic_identifier.rb | 10 ++- .../matchers/link_handlers/work_packages.rb | 23 +++--- .../matchers/resource_links_matcher.rb | 78 +++++++++---------- .../link_handlers/work_packages_spec.rb | 8 +- 4 files changed, 62 insertions(+), 57 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 4dbd5dc76f2..aeb1025bf66 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -137,10 +137,12 @@ module WorkPackage::SemanticIdentifier WorkPackage::SemanticIdentifier.format(display_id) end - # Module-level variant for callers that already hold a display id and - # don't need the WorkPackage record. - def self.format(display_id) - display_id.is_a?(String) && display_id.match?(/[A-Za-z]/) ? display_id : "##{display_id}" + # Module-level variant of `#formatted_id` for callers that already hold + # an identifier value (which may or may not be semantic) and don't need + # the WorkPackage record. Semantic shapes pass through unchanged; + # numeric shapes get the classic `#N` prefix. + def self.format(value) + value.is_a?(String) && value.match?(/[A-Za-z]/) ? value : "##{value}" end # Override ActiveRecord's default `to_param` so Rails URL helpers 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 755e67c1dd1..5926c297e41 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,7 +78,7 @@ 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? @@ -89,7 +89,7 @@ module OpenProject::TextFormatting::Matchers 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? render_work_package_macro(work_package: wp, fallback_id: wp_id, detailed: detailed?) @@ -103,8 +103,7 @@ module OpenProject::TextFormatting::Matchers display_id = work_package&.display_id || fallback_id label = WorkPackage::SemanticIdentifier.format(display_id) - return label if context[:plain_text] - return label if work_package && !visible_to_current_user?(work_package.id) + return label if text_only?(work_package) ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", @@ -116,8 +115,7 @@ module OpenProject::TextFormatting::Matchers # render path bypassing `PatternMatcherFilter`) rather than running a # per-link query inside the renderer. label = work_package&.formatted_id || "##{fallback_id}" - return label if context[:plain_text] - return label if work_package && !visible_to_current_user?(work_package.id) + return label if text_only?(work_package) href_id = work_package&.display_id || fallback_id @@ -130,9 +128,16 @@ module OpenProject::TextFormatting::Matchers }) end - def visible_to_current_user?(work_package_id) - OpenProject::TextFormatting::Matchers::ResourceLinksMatcher - .visible_to_current_user?(work_package_id) + # Plain-text channels and inaccessible WPs both render the label + # without an anchor or quickinfo. Visibility is checked only when a + # WP was preloaded — a nil work_package means a classic-mode render + # or an unresolved reference, neither of which needs 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 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 e558eb677b9..e5574ea8ef0 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -69,9 +69,32 @@ module OpenProject::TextFormatting # identifier:version:1.0.0 # identifier:source:some/file class ResourceLinksMatcher < RegexMatcher - WORK_PACKAGES_LOOKUP_KEY = :text_formatting_work_packages_lookup - VISIBLE_WORK_PACKAGE_IDS_KEY = :text_formatting_visible_work_package_ids - private_constant :WORK_PACKAGES_LOOKUP_KEY, :VISIBLE_WORK_PACKAGE_IDS_KEY + # Per-render preload state shared between the matcher and the link + # handler. Pairs unscoped label resolution (`lookup`) with viewer-scoped + # link gating (`visible_ids`) so a single render computes both with a + # bounded number of round-trips and renders consistent labels for + # invisible WPs. + 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 + + 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 @@ -131,57 +154,31 @@ 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) - end - - # The main lookup is unscoped so labels resolve regardless of - # current-user permissions. This predicate gates whether the - # link handler emits a navigable anchor or a plain-text label. - def self.visible_to_current_user?(work_package_id) - RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY]&.include?(work_package_id) || false + # The active preload cache for the current render, or an empty + # singleton when no preload is in scope (classic mode, no `#N` + # references, or rendering outside `with_preloaded_resources`). + 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 + # the cache 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 = stash_preload_state + previous = RequestStore.store[CACHE_KEY] return yield unless Setting::WorkPackageIdentifier.semantic? identifiers = collect_work_package_identifiers(doc) return yield if identifiers.empty? - install_preload_state(*build_lookup(identifiers)) + RequestStore.store[CACHE_KEY] = build_cache(identifiers) yield ensure - restore_preload_state(previous) + RequestStore.store[CACHE_KEY] = previous end - def self.stash_preload_state - [RequestStore.store[WORK_PACKAGES_LOOKUP_KEY], - RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY]] - end - private_class_method :stash_preload_state - - def self.install_preload_state(lookup, visible_ids) - RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = lookup - RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY] = visible_ids - end - private_class_method :install_preload_state - - def self.restore_preload_state(previous) - RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = previous[0] - RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY] = previous[1] - end - private_class_method :restore_preload_state - def self.collect_work_package_identifiers(doc) identifiers = Set.new doc.search(".//text()").each do |node| @@ -218,14 +215,15 @@ module OpenProject::TextFormatting # for historical aliases — the loaded row carries only the current # identifier, so unmapped inputs are filled in from # `WorkPackageSemanticAlias`. - def self.build_lookup(identifiers) + def self.build_cache(identifiers) work_packages = WorkPackage.where_display_id_in(*identifiers).select(:id, :identifier).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, all_wp_ids:) - [lookup, visible_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 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 14a2a1cbbc4..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 From 5d8929a7c34570f2a4615ab6ae71f52d086e4851 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 25 May 2026 16:32:12 +0300 Subject: [PATCH 04/20] Resolve invisible WP mentions to their current formatted_id The mention filter previously dropped to the envelope's stored text when the recipient lacked view permission on the referenced work package, which left stale identifiers in mailer bodies after a project rename and diverged from the `#N` text-reference path on the same render. Adopts the two-SELECT pattern ResourceLinksMatcher uses for `#N` references: a single unscoped batched lookup for label resolution plus a visibility-scoped id pluck for anchor gating. Invisible WPs render as plain text with the current formatted_id; the per-mention `WorkPackage.visible.find_by` is gone. --- .../text_formatting/filters/mention_filter.rb | 54 +++++++++++++------ .../filters/mention_filter_spec.rb | 32 +++++++++++ 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 9c2740d63e8..14cc26048e7 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_work_package_mentions + doc.search("mention").each do |mention| anchor = mention_anchor(mention) mention.replace(anchor) if anchor @@ -47,6 +49,22 @@ module OpenProject::TextFormatting private + # Bounded two-SELECT preload for WP mentions: an unscoped fetch (label + # resolution regardless of viewer) plus a visibility-scoped id pluck + # (gate anchor vs plain-text render). Mirrors the pattern + # `ResourceLinksMatcher` uses for `#N` text references so mentions + # and references resolve to the same shape for the same recipient, + # and avoids the per-mention query the old `.visible.find_by` did. + def preload_work_package_mentions + ids = mention_work_package_ids + @mentioned_work_packages = ids.empty? ? {} : WorkPackage.where(id: ids).index_by(&:id) + @visible_mentioned_ids = ids.empty? ? Set.new : WorkPackage.visible.where(id: ids).pluck(:id).to_set + end + + def mention_work_package_ids + doc.css('mention[data-type="work_package"]').filter_map { |m| mention_id(m)&.to_i }.uniq + end + def mention_anchor(mention) mention_instance = class_from_mention(mention) @@ -75,7 +93,7 @@ module OpenProject::TextFormatting end def work_package_mention(work_package, mention) - return Nokogiri::XML::Text.new(work_package.formatted_id, mention.document) if context[:plain_text] + return Nokogiri::XML::Text.new(work_package.formatted_id, mention.document) if text_only?(work_package) # Match the label and URL convention used for `#N` text references # elsewhere in the markdown pipeline. @@ -86,6 +104,14 @@ module OpenProject::TextFormatting end end + # Plain-text channels and inaccessible WPs both render the + # `formatted_id` without an anchor or quickinfo widget — the + # latter would resolve to a hover-card endpoint the recipient + # can't reach. + def text_only?(work_package) + context[:plain_text] || @visible_mentioned_ids.exclude?(work_package.id) + end + def work_package_quickinfo(work_package, detailed:) ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", @@ -105,21 +131,19 @@ module OpenProject::TextFormatting }) end + # WP label resolution is unscoped (preloaded above); visibility is + # gated separately in `text_only?` so an inaccessible WP renders + # its current formatted_id rather than the envelope text the + # author originally typed. User/group mentions stay visibility-gated + # at the find — there's no equivalent text-only render path for + # principals. 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 - - mention_class - .visible - .find_by(id: mention_id(mention)) || fallback_text(mention) + case mention.attributes["data-type"].value + when "user" then User.visible.find_by(id: mention_id(mention)) + when "group" then Group.visible.find_by(id: mention_id(mention)) + when "work_package" then @mentioned_work_packages[mention_id(mention)&.to_i] + else raise ArgumentError + end || fallback_text(mention) end ## diff --git a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb index e85c666fa36..6205a4f6938 100644 --- a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb +++ b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb @@ -171,6 +171,38 @@ RSpec.describe OpenProject::TextFormatting::Filters::MentionFilter do end end + context "with a mention to an inaccessible WP", + with_flag: { semantic_work_package_ids: true }, + with_settings: { work_packages_identifier: "semantic" } do + # Label resolution is unscoped so the envelope renders the WP's + # current `formatted_id` (e.g. `HIDDEN-1`) rather than the literal + # envelope text the author originally typed — keeps the mention + # path consistent with `#N` text references in the same render. + let(:project) { create(:project, identifier: "VISIBLE") } + let(:hidden_project) { create(:project, identifier: "HIDDEN") } + let(:hidden_wp) { create(:work_package, project: hidden_project) } + + before { hidden_wp.allocate_and_register_semantic_id } + + it "renders the formatted_id as plain text with no anchor or quickinfo" do + wp = hidden_wp.reload + rendered = format_text(mention_tag(wp)) + + 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 + + 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 + # Semantic-shaped data-ids must not silently resolve to a WP whose id # matches the embedded digits. context "with a semantic-shaped data-id whose embedded digits collide with a real WP id", From a54dd3dc0b01f913e1ccc041fcfe22cb2ad80525 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 25 May 2026 16:33:23 +0300 Subject: [PATCH 05/20] Rename :plain_text format to :markdown_as_text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The format runs the full markdown pipeline and then collapses the DOM to text — it has nothing to do with the existing `:plain` format, which strips markdown entirely. Moves the formatter under the Markdown namespace next to the rich-output formatter whose pipeline it mirrors, and renames the symbol so the relationship is legible from the formatter_for case clause. --- .../work_package_mailer/mentioned.text.erb | 2 +- .../watcher_changed.text.erb | 2 +- .../{plain => markdown}/text_formatter.rb | 16 ++++++++------ lib/open_project/text_formatting/renderer.rb | 6 ++--- .../filters/mention_filter_spec.rb | 22 +++++++++++++++++++ .../text_formatter_spec.rb | 2 +- 6 files changed, 37 insertions(+), 13 deletions(-) rename lib/open_project/text_formatting/formats/{plain => markdown}/text_formatter.rb (82%) rename spec/lib/open_project/text_formatting/formats/{plain => markdown}/text_formatter_spec.rb (98%) diff --git a/app/views/work_package_mailer/mentioned.text.erb b/app/views/work_package_mailer/mentioned.text.erb index 45a2c8766e4..5b8ec48d8ca 100644 --- a/app/views/work_package_mailer/mentioned.text.erb +++ b/app/views/work_package_mailer/mentioned.text.erb @@ -10,7 +10,7 @@ <%= I18n.t(:label_comment_added) %>: <%= format_text( @journal.notes, - format: :plain_text, + format: :markdown_as_text, 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 6dceb846e5e..8707c958f6c 100644 --- a/app/views/work_package_mailer/watcher_changed.text.erb +++ b/app/views/work_package_mailer/watcher_changed.text.erb @@ -33,7 +33,7 @@ See COPYRIGHT and LICENSE files for more details. <%= render partial: "work_package_details", locals: { work_package: @work_package } %> <%= format_text( t(:text_latest_note, note: last_work_package_note(@work_package)), - format: :plain_text, + format: :markdown_as_text, only_path: false, object: @work_package, project: @work_package.project diff --git a/lib/open_project/text_formatting/formats/plain/text_formatter.rb b/lib/open_project/text_formatting/formats/markdown/text_formatter.rb similarity index 82% rename from lib/open_project/text_formatting/formats/plain/text_formatter.rb rename to lib/open_project/text_formatting/formats/markdown/text_formatter.rb index 8dfb25ac19c..a37d566f233 100644 --- a/lib/open_project/text_formatting/formats/plain/text_formatter.rb +++ b/lib/open_project/text_formatting/formats/markdown/text_formatter.rb @@ -29,12 +29,12 @@ #++ module OpenProject::TextFormatting::Formats - module Plain - # Plain-text output sibling of `Plain::Formatter`. Shares the matcher - # and mention pipeline with the rich (markdown) renderer so identifier - # resolution stays consistent across channels, but collapses the final - # DOM to text. Intended for plain/text mailers and similar contexts - # where HTML would be a foreign body. + module Markdown + # Text-output sibling of `Markdown::Formatter`. Shares the matcher and + # mention pipeline with the rich renderer so identifier resolution + # stays consistent across channels, then collapses the final DOM to + # text via `PlainTextOutputFilter`. Intended for plain/text mailers + # and other channels where HTML would be a foreign body. class TextFormatter < OpenProject::TextFormatting::Formats::BaseFormatter def initialize(context) super(context.merge(plain_text: true)) @@ -55,7 +55,9 @@ module OpenProject::TextFormatting::Formats ] end - def self.format = :plain_text + def self.format + :markdown_as_text + end end end end diff --git a/lib/open_project/text_formatting/renderer.rb b/lib/open_project/text_formatting/renderer.rb index e251f0cd894..d8f29bfd70e 100644 --- a/lib/open_project/text_formatting/renderer.rb +++ b/lib/open_project/text_formatting/renderer.rb @@ -49,14 +49,14 @@ module OpenProject::TextFormatting .to_html(text) end - # @param [:plain, :plain_text, :rich] format the text format. + # @param [:plain, :markdown_as_text, :rich] format the text format. # @return [Formats::BaseFormatter] a formatter implementation. def formatter_for(format) case format.to_sym when :plain Formats.plain_formatter - when :plain_text - Formats::Plain::TextFormatter + when :markdown_as_text + Formats::Markdown::TextFormatter else Formats.rich_formatter end diff --git a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb index 6205a4f6938..a0b6268516d 100644 --- a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb +++ b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb @@ -203,6 +203,28 @@ RSpec.describe OpenProject::TextFormatting::Filters::MentionFilter do end end + context "in plain-text rendering mode", + with_flag: { semantic_work_package_ids: true }, + with_settings: { work_packages_identifier: "semantic" } do + # The `:markdown_as_text` channel must collapse mention envelopes + # to their current `formatted_id` so the plain-text 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), format: :markdown_as_text) + + expect(rendered).to include(wp.formatted_id) + expect(rendered).not_to include(" Date: Tue, 26 May 2026 12:43:27 +0300 Subject: [PATCH 06/20] Cleanup comments and order static vs instance method locations --- app/models/work_package/semantic_identifier.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index aeb1025bf66..ddd75641062 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(value) + value.is_a?(String) && value.match?(/[A-Za-z]/) ? value : "##{value}" + 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 @@ -137,14 +144,6 @@ module WorkPackage::SemanticIdentifier WorkPackage::SemanticIdentifier.format(display_id) end - # Module-level variant of `#formatted_id` for callers that already hold - # an identifier value (which may or may not be semantic) and don't need - # the WorkPackage record. Semantic shapes pass through unchanged; - # numeric shapes get the classic `#N` prefix. - def self.format(value) - value.is_a?(String) && value.match?(/[A-Za-z]/) ? value : "##{value}" - end - # Override ActiveRecord's default `to_param` so Rails URL helpers # (work_package_path, polymorphic_path, form_for, etc.) automatically # produce semantic-id URLs in semantic mode. In classic mode display_id From ee4c9aee59cb7e9494a595beb0487e617d144a7f Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 12:51:00 +0300 Subject: [PATCH 07/20] Render WP quickinfo macros as static HTML in mailer notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `##N` and `###N` work-package macros emit JS-hydrated `` custom elements, which mail clients collapse to empty bullets. Introduce a `:markdown_as_static_html` format that shares the rich filter chain but signals `context[:as_static_html]` so the matcher and `MentionFilter` emit a server-rendered anchor — formatted_id, type name, subject, and (for `###`) status name — closely mirroring the in-app widget once flattened. Mailer HTML templates (`mentioned`, `watcher_changed`, `_work_package_details`) opt into the new format. Invisible WPs still render as plain-text labels, matching the cross-project visibility policy. `ResourceLinksMatcher.build_cache` and `MentionFilter#preload_work_package_mentions` eager-load `:type` and `:status` only when `:as_static_html` is set, leaving the default web path's two-SELECT shape untouched. Classic-mode preload now also runs under `:as_static_html` so the link handler can resolve type/subject for `##`/`###`. Renames the internal flag `context[:plain_text]` to `context[:as_text]` to restore symmetry with the user-facing `:markdown_as_text` format. --- .../_work_package_details.html.erb | 2 +- .../work_package_mailer/mentioned.html.erb | 2 +- .../watcher_changed.html.erb | 1 + .../text_formatting/filters/mention_filter.rb | 33 +++- .../filters/plain_text_output_filter.rb | 2 +- .../formats/markdown/static_html_formatter.rb | 50 +++++ .../formats/markdown/text_formatter.rb | 2 +- .../matchers/link_handlers/work_packages.rb | 22 ++- .../matchers/resource_links_matcher.rb | 31 ++- lib/open_project/text_formatting/renderer.rb | 4 +- .../markdown/static_html_formatter_spec.rb | 180 ++++++++++++++++++ spec/mailers/work_package_mailer_spec.rb | 58 ++++++ 12 files changed, 369 insertions(+), 18 deletions(-) create mode 100644 lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb create mode 100644 spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb 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..5f53d3e9ef9 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_text(work_package, :description, format: :markdown_as_static_html, only_path: false) %> diff --git a/app/views/work_package_mailer/mentioned.html.erb b/app/views/work_package_mailer/mentioned.html.erb index 69831d69f34..eda3d8e4fb2 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_text @journal.notes, format: :markdown_as_static_html, only_path: false %>
diff --git a/app/views/work_package_mailer/watcher_changed.html.erb b/app/views/work_package_mailer/watcher_changed.html.erb index 6089c2dd7b9..2275780822e 100644 --- a/app/views/work_package_mailer/watcher_changed.html.erb +++ b/app/views/work_package_mailer/watcher_changed.html.erb @@ -32,6 +32,7 @@ See COPYRIGHT and LICENSE files for more details.

<%= format_text( t(:text_latest_note, note: last_work_package_note(@work_package)), + format: :markdown_as_static_html, only_path: false, object: @work_package, project: @work_package.project diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 14cc26048e7..6730542395e 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -57,8 +57,18 @@ module OpenProject::TextFormatting # and avoids the per-mention query the old `.visible.find_by` did. def preload_work_package_mentions ids = mention_work_package_ids - @mentioned_work_packages = ids.empty? ? {} : WorkPackage.where(id: ids).index_by(&:id) - @visible_mentioned_ids = ids.empty? ? Set.new : WorkPackage.visible.where(id: ids).pluck(:id).to_set + if ids.empty? + @mentioned_work_packages = {} + @visible_mentioned_ids = Set.new + return + end + + scope = WorkPackage.where(id: ids) + # Static-HTML channels need `type` and `status` to render + # quickinfo envelopes as anchors instead of `` widgets. + scope = scope.includes(:type, :status) if context[:as_static_html] + @mentioned_work_packages = scope.index_by(&:id) + @visible_mentioned_ids = WorkPackage.visible.where(id: ids).pluck(:id).to_set end def mention_work_package_ids @@ -109,10 +119,12 @@ module OpenProject::TextFormatting # latter would resolve to a hover-card endpoint the recipient # can't reach. def text_only?(work_package) - context[:plain_text] || @visible_mentioned_ids.exclude?(work_package.id) + context[:as_text] || @visible_mentioned_ids.exclude?(work_package.id) end def work_package_quickinfo(work_package, detailed:) + return work_package_static_macro(work_package, detailed:) if context[:as_static_html] + ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", data: { id: work_package.id, @@ -120,6 +132,21 @@ module OpenProject::TextFormatting detailed: } end + # Static fallback shared with the PatternMatcherFilter's `##`/`###` + # path so envelope-driven and text-driven references render the same + # shape in channels that cannot hydrate the custom element. + def work_package_static_macro(work_package, detailed:) + parts = [] + parts << work_package.status&.name if detailed + parts << work_package.type&.name + parts << work_package.formatted_id + link_text = "#{parts.compact.join(' ')}: #{work_package.subject}" + + link_to(link_text, + 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, 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 index fb3b0511059..b23c992af0e 100644 --- a/lib/open_project/text_formatting/filters/plain_text_output_filter.rb +++ b/lib/open_project/text_formatting/filters/plain_text_output_filter.rb @@ -32,7 +32,7 @@ module OpenProject::TextFormatting module Filters # Final stage of the plain-text pipeline. Earlier filters resolve # mentions and macros to their text-mode shapes (driven by - # `context[:plain_text]`); this stage collapses any remaining markup + # `context[:as_text]`); this stage collapses any remaining markup # so the pipeline output is suitable for `text/plain` bodies. class PlainTextOutputFilter < HTML::Pipeline::Filter def call diff --git a/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb b/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb new file mode 100644 index 00000000000..dcf25ca8b5a --- /dev/null +++ b/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb @@ -0,0 +1,50 @@ +# 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::Formats + module Markdown + # Static-HTML sibling of `Markdown::Formatter`. Shares the same filter + # chain so identifier resolution, mention handling, and link rendering + # stay consistent, but signals `context[:as_static_html]` so matchers + # and filters emit server-rendered anchors in place of JS-hydrated + # custom elements. Intended for channels that cannot run JS — HTML + # mailers, server-side previews, archival exports — where dynamic + # widgets would collapse to empty placeholders. + class StaticHtmlFormatter < Formatter + def initialize(context) + super(context.merge(as_static_html: true)) + end + + def self.format + :markdown_as_static_html + end + end + end +end diff --git a/lib/open_project/text_formatting/formats/markdown/text_formatter.rb b/lib/open_project/text_formatting/formats/markdown/text_formatter.rb index a37d566f233..b320fb967e3 100644 --- a/lib/open_project/text_formatting/formats/markdown/text_formatter.rb +++ b/lib/open_project/text_formatting/formats/markdown/text_formatter.rb @@ -37,7 +37,7 @@ module OpenProject::TextFormatting::Formats # and other channels where HTML would be a foreign body. class TextFormatter < OpenProject::TextFormatting::Formats::BaseFormatter def initialize(context) - super(context.merge(plain_text: true)) + super(context.merge(as_text: true)) end def to_html(text) 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 5926c297e41..b1d7f706516 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 @@ -104,12 +104,32 @@ module OpenProject::TextFormatting::Matchers label = WorkPackage::SemanticIdentifier.format(display_id) return label if text_only?(work_package) + return render_static_work_package_macro(work_package, label, detailed:) if context[:as_static_html] ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", data: { id:, display_id:, detailed: } end + # Static fallback for channels that cannot hydrate the quickinfo + # custom element (HTML mailers, exports). Mirrors the in-app widget's + # text composition — type, optional status, formatted_id, subject — + # so the anchor reads the same as the rich rendering once flattened. + # Unresolved references collapse to the bare label. + def render_static_work_package_macro(work_package, label, detailed:) + return label unless work_package + + parts = [] + parts << work_package.status&.name if detailed + parts << work_package.type&.name + parts << label + link_text = "#{parts.compact.join(' ')}: #{work_package.subject}" + + link_to(link_text, + 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 @@ -133,7 +153,7 @@ module OpenProject::TextFormatting::Matchers # WP was preloaded — a nil work_package means a classic-mode render # or an unresolved reference, neither of which needs gating. def text_only?(work_package) - context[:plain_text] || (work_package && !preload_cache.visible?(work_package.id)) + context[:as_text] || (work_package && !preload_cache.visible?(work_package.id)) end def preload_cache 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 e5574ea8ef0..c7a420ffa77 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -163,17 +163,19 @@ module OpenProject::TextFormatting # Doc-level preload called by `PatternMatcherFilter`. Save/restores # the cache 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) + # re-entering the pipeline) doesn't clobber the outer render. + # Classic mode normally skips the load (the link handler renders + # `#N` from the matched id alone), but static-HTML channels need + # the WP record in both modes to compose the type/subject/status + # anchor. + def self.with_preloaded_resources(doc, context) previous = RequestStore.store[CACHE_KEY] - return yield unless Setting::WorkPackageIdentifier.semantic? + return yield unless Setting::WorkPackageIdentifier.semantic? || context[:as_static_html] identifiers = collect_work_package_identifiers(doc) return yield if identifiers.empty? - RequestStore.store[CACHE_KEY] = build_cache(identifiers) + RequestStore.store[CACHE_KEY] = build_cache(identifiers, context) yield ensure RequestStore.store[CACHE_KEY] = previous @@ -214,9 +216,20 @@ module OpenProject::TextFormatting # one visibility-scoped id pluck. A third targeted SELECT fires # for historical aliases — the loaded row carries only the current # identifier, so unmapped inputs are filled in from - # `WorkPackageSemanticAlias`. - def self.build_cache(identifiers) - work_packages = WorkPackage.where_display_id_in(*identifiers).select(:id, :identifier).to_a + # `WorkPackageSemanticAlias`. Static-HTML channels also eager-load + # `:type` and `:status` so the link handler can render the + # static-anchor variant of `##`/`###` macros without N+1 queries — + # those associations are the metadata a reader needs to recognise a + # WP reference flattened to text. Anything beyond that (project, + # versions, custom fields) stays out of this preload. + def self.build_cache(identifiers, context = {}) + scope = WorkPackage.where_display_id_in(*identifiers) + scope = if context[:as_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) diff --git a/lib/open_project/text_formatting/renderer.rb b/lib/open_project/text_formatting/renderer.rb index d8f29bfd70e..68b9e6187ad 100644 --- a/lib/open_project/text_formatting/renderer.rb +++ b/lib/open_project/text_formatting/renderer.rb @@ -49,7 +49,7 @@ module OpenProject::TextFormatting .to_html(text) end - # @param [:plain, :markdown_as_text, :rich] format the text format. + # @param [:plain, :markdown_as_text, :markdown_as_static_html, :rich] format the text format. # @return [Formats::BaseFormatter] a formatter implementation. def formatter_for(format) case format.to_sym @@ -57,6 +57,8 @@ module OpenProject::TextFormatting Formats.plain_formatter when :markdown_as_text Formats::Markdown::TextFormatter + when :markdown_as_static_html + Formats::Markdown::StaticHtmlFormatter else Formats.rich_formatter end diff --git a/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb b/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb new file mode 100644 index 00000000000..4dc9232ad71 --- /dev/null +++ b/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb @@ -0,0 +1,180 @@ +# 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::Formats::Markdown::StaticHtmlFormatter do + subject(:formatted) { described_class.new(context).to_html(input) } + + 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_flag: { semantic_work_package_ids: true }, + 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_flag: { semantic_work_package_ids: true }, + 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_flag: { semantic_work_package_ids: false }, + 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_flag: { semantic_work_package_ids: true }, + 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_flag: { semantic_work_package_ids: false }, + 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_flag: { semantic_work_package_ids: true }, + 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 = described_class.new(context).to_html("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 = described_class.new(context).to_html("see #{'###'}#{hidden_wp.id}") + expect(rendered).to include("SECRET-1") + expect(rendered).not_to match(%r{]*>[^<]*SECRET-1}) + 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_flag: { semantic_work_package_ids: true }, + 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 + + describe "format identifier" do + it "exposes :markdown_as_static_html" do + expect(described_class.format).to eq(:markdown_as_static_html) + end + end + + describe "renderer routing" do + it "Renderer.formatter_for(:markdown_as_static_html) resolves to this class" do + expect(OpenProject::TextFormatting::Renderer.formatter_for(:markdown_as_static_html)) + .to eq(described_class) + end + end +end diff --git a/spec/mailers/work_package_mailer_spec.rb b/spec/mailers/work_package_mailer_spec.rb index 64df0a68ab5..017aa2811cb 100644 --- a/spec/mailers/work_package_mailer_spec.rb +++ b/spec/mailers/work_package_mailer_spec.rb @@ -324,5 +324,63 @@ RSpec.describe WorkPackageMailer do 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_flag: { semantic_work_package_ids: true }, + 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_flag: { semantic_work_package_ids: false }, + 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 From e269fddffce9977edbfd2290960365c9c480aa68 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 12:54:40 +0300 Subject: [PATCH 08/20] Extract shared static-macro label composition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer feedback on cd122f867cc: the `parts << status / type / label` block was duplicated between the regex-driven (`WorkPackages` link handler) and envelope-driven (`MentionFilter`) static paths, with no guard against silent desynchronisation. Centralise the composition on the link handler and document why the two callers pass different labels — the regex path preserves the alias-as-matched, the envelope path normalises to the WP's current formatted_id. --- .../text_formatting/filters/mention_filter.rb | 13 ++++----- .../matchers/link_handlers/work_packages.rb | 29 ++++++++++++------- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 6730542395e..7f11e8bfd45 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -134,15 +134,14 @@ module OpenProject::TextFormatting # Static fallback shared with the PatternMatcherFilter's `##`/`###` # path so envelope-driven and text-driven references render the same - # shape in channels that cannot hydrate the custom element. + # shape in channels that cannot hydrate the custom element. Always + # uses the WP's current `formatted_id`, normalising any historical + # alias the envelope may have been authored with. def work_package_static_macro(work_package, detailed:) - parts = [] - parts << work_package.status&.name if detailed - parts << work_package.type&.name - parts << work_package.formatted_id - link_text = "#{parts.compact.join(' ')}: #{work_package.subject}" + label = OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages + .compose_static_macro_label(work_package, label: work_package.formatted_id, detailed:) - link_to(link_text, + link_to(label, work_package_path_or_url(id: work_package.display_id, only_path: context[:only_path]), class: "issue work_package") 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 b1d7f706516..09c41d04bdf 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 @@ -38,6 +38,17 @@ module OpenProject::TextFormatting::Matchers # Shared with the PDF-export subclass in `app/models/work_package/exports/macros/links.rb`. HASH_TRIGGERS = %w[# ## ###].freeze + # Builds the user-facing label for the static-anchor variant of `##`/ + # `###` macros. Centralised so the PatternMatcherFilter path here and + # the ``-envelope path in `MentionFilter` stay in lockstep. + def self.compose_static_macro_label(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 + def applicable? hash_trigger? && matcher.prefix.nil? end @@ -112,20 +123,16 @@ module OpenProject::TextFormatting::Matchers end # Static fallback for channels that cannot hydrate the quickinfo - # custom element (HTML mailers, exports). Mirrors the in-app widget's - # text composition — type, optional status, formatted_id, subject — - # so the anchor reads the same as the rich rendering once flattened. - # Unresolved references collapse to the bare label. + # custom element. Composes type, optional status, label, and subject + # into the anchor; unresolved references collapse to the bare label. + # The label is the matched identifier (potentially a historical alias) + # to preserve what the author wrote — the `` envelope path + # in `MentionFilter` instead normalises to the WP's current + # `formatted_id`. def render_static_work_package_macro(work_package, label, detailed:) return label unless work_package - parts = [] - parts << work_package.status&.name if detailed - parts << work_package.type&.name - parts << label - link_text = "#{parts.compact.join(' ')}: #{work_package.subject}" - - link_to(link_text, + link_to(self.class.compose_static_macro_label(work_package, label:, detailed:), work_package_path_or_url(id: work_package.display_id, only_path: context[:only_path]), class: "issue work_package") end From 666069e1263a6edde30ada1fa6e5cd2c9cfc047d Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 13:40:29 +0300 Subject: [PATCH 09/20] Cleanup unneeded comments, touch up syntax --- lib/open_project/text_formatting/filters/mention_filter.rb | 2 +- .../text_formatting/matchers/link_handlers/work_packages.rb | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 7f11e8bfd45..3fadec73ada 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -72,7 +72,7 @@ module OpenProject::TextFormatting end def mention_work_package_ids - doc.css('mention[data-type="work_package"]').filter_map { |m| mention_id(m)&.to_i }.uniq + doc.css('mention[data-type="work_package"]').filter_map { mention_id(it)&.to_i }.uniq end def mention_anchor(mention) 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 09c41d04bdf..42e8ab769b1 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 @@ -38,9 +38,6 @@ module OpenProject::TextFormatting::Matchers # Shared with the PDF-export subclass in `app/models/work_package/exports/macros/links.rb`. HASH_TRIGGERS = %w[# ## ###].freeze - # Builds the user-facing label for the static-anchor variant of `##`/ - # `###` macros. Centralised so the PatternMatcherFilter path here and - # the ``-envelope path in `MentionFilter` stay in lockstep. def self.compose_static_macro_label(work_package, label:, detailed:) parts = [] parts << work_package.status&.name if detailed From 6e3068531fe74dd11ab61009dd60a6f0c35d82bd Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 14:15:39 +0300 Subject: [PATCH 10/20] Write primary-key WP refs in auto-generated journal notes The earlier write-time canonicalization stored the rendered display_id ("#PROJ-7") in the journal note, which would rot under project-identifier renames and leave dangling semantic strings if semantic mode were rolled back. Restore the PK shape ("#42") and let the formatter pipeline turn it into the user-facing identifier at render time, where the resolver already handles both modes. Both spec contexts now assert the same PK shape; the mode-specific rendering of "#N" lives in the formatter specs. --- .../work_packages/update_ancestors_service.rb | 2 +- .../work_packages/update_ancestors_service_spec.rb | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index 966766ee3b3..9466248c8bb 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -124,7 +124,7 @@ 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.display_id}") + child: "##{initiator_work_package.id}") end end diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index a18e3c44d3f..baf6321e6fd 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -1394,10 +1394,14 @@ RSpec.describe WorkPackages::UpdateAncestorsService, 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 identifier renames and feature-flag rollback. context "in classic mode", with_flag: { semantic_work_package_ids: false }, with_settings: { work_package_done_ratio: "status", work_packages_identifier: "classic" } do - it "writes the child's hash-prefixed numeric id into the parent's journal note" 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) @@ -1413,14 +1417,14 @@ RSpec.describe WorkPackages::UpdateAncestorsService, child.allocate_and_register_semantic_id end - it "writes the child's hash-prefixed semantic identifier into the parent's journal note" do + 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.display_id}") - expect(note).not_to match(/##{wp.id}\b/) + expect(note).to include("##{wp.id}") + expect(note).not_to include(wp.identifier) end end end From e70b9ab21c8c10663b23adcaf0a233bc46069ed5 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 14:15:58 +0300 Subject: [PATCH 11/20] Tighten mention pipeline: helper extraction, principal preload, coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lift the static-anchor label composition out of LinkHandlers and into a small Helpers::StaticMacroLabel module so the envelope path (MentionFilter) and the text-reference path (LinkHandlers) share one shape — same module called from both, no cross-class reach-through. Batch the User and Group mention preloads alongside the existing WP preload so a note with N principals costs one SELECT per type rather than N. Class lookup now reads from indexed hashes; visibility-gating stays where it was (at the find for principals, separate from the label for WPs). Rename SemanticIdentifier.format → with_hash_prefix; the prior name was broad enough to invite misuse for arbitrary work-package values. Override StaticHtmlFormatter#filters explicitly so a future filter appearing in Formatter#filters is a deliberate decision to apply to mailer-side rendering, not an automatic one. Spec coverage: classic-mode quickinfo and inaccessible-WP paths (symmetric with the existing semantic-mode contexts), a principal preload N+1 guard, and an anonymous current_user smoke test that confirms the static-HTML pipeline doesn't raise when invoked without an authenticated viewer. --- .../work_package/semantic_identifier.rb | 4 +- .../text_formatting/filters/mention_filter.rb | 54 +++++++++++------ .../helpers/static_macro_label.rb | 53 +++++++++++++++++ .../matchers/link_handlers/work_packages.rb | 16 ++--- .../matchers/resource_links_matcher.rb | 11 ++-- .../filters/mention_filter_spec.rb | 45 ++++++++++++++ .../markdown/static_html_formatter_spec.rb | 59 +++++++++++++++++++ .../formats/markdown/text_formatter_spec.rb | 14 +++++ 8 files changed, 220 insertions(+), 36 deletions(-) create mode 100644 lib/open_project/text_formatting/helpers/static_macro_label.rb diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index ddd75641062..b8b728a9069 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -124,7 +124,7 @@ module WorkPackage::SemanticIdentifier # Returns formatted value for inline UI display. # * Semantic mode: "PROJ-42" (no prefix — self-describing) # * Classic mode: "#42" (hash-prefixed) - def self.format(value) + def self.with_hash_prefix(value) value.is_a?(String) && value.match?(/[A-Za-z]/) ? value : "##{value}" end @@ -141,7 +141,7 @@ module WorkPackage::SemanticIdentifier # Semantic mode: "PROJ-42" (no prefix — self-describing) # Classic mode: "#42" (hash-prefixed) def formatted_id - WorkPackage::SemanticIdentifier.format(display_id) + WorkPackage::SemanticIdentifier.with_hash_prefix(display_id) end # Override ActiveRecord's default `to_param` so Rails URL helpers diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 3fadec73ada..9ddef4e1285 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -37,7 +37,7 @@ module OpenProject::TextFormatting include OpenProject::StaticRouting::UrlHelpers def call - preload_work_package_mentions + preload_mentions doc.search("mention").each do |mention| anchor = mention_anchor(mention) @@ -49,14 +49,20 @@ module OpenProject::TextFormatting private - # Bounded two-SELECT preload for WP mentions: an unscoped fetch (label - # resolution regardless of viewer) plus a visibility-scoped id pluck - # (gate anchor vs plain-text render). Mirrors the pattern - # `ResourceLinksMatcher` uses for `#N` text references so mentions - # and references resolve to the same shape for the same recipient, - # and avoids the per-mention query the old `.visible.find_by` did. + # Doc-level preload so a note with N user/group/WP mentions costs + # a bounded number of SELECTs rather than one per mention. WP + # mentions need the unscoped record (label) plus a separate + # visibility id pluck (anchor-vs-text gating); principals fold the + # two concerns into a single visibility-scoped fetch since + # invisible principals have no plain-text render path — they fall + # back to the literal envelope text instead. + def preload_mentions + preload_work_package_mentions + preload_principal_mentions + end + def preload_work_package_mentions - ids = mention_work_package_ids + ids = mention_ids_for("work_package") if ids.empty? @mentioned_work_packages = {} @visible_mentioned_ids = Set.new @@ -71,8 +77,15 @@ module OpenProject::TextFormatting @visible_mentioned_ids = WorkPackage.visible.where(id: ids).pluck(:id).to_set end - def mention_work_package_ids - doc.css('mention[data-type="work_package"]').filter_map { mention_id(it)&.to_i }.uniq + 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) @@ -117,7 +130,9 @@ module OpenProject::TextFormatting # Plain-text channels and inaccessible WPs both render the # `formatted_id` without an anchor or quickinfo widget — the # latter would resolve to a hover-card endpoint the recipient - # can't reach. + # can't reach. Mirrors `LinkHandlers::WorkPackages#text_only?`, + # which gates the matching decision for `#N` text references; keep + # the two in sync. def text_only?(work_package) context[:as_text] || @visible_mentioned_ids.exclude?(work_package.id) end @@ -138,8 +153,8 @@ module OpenProject::TextFormatting # uses the WP's current `formatted_id`, normalising any historical # alias the envelope may have been authored with. def work_package_static_macro(work_package, detailed:) - label = OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages - .compose_static_macro_label(work_package, label: work_package.formatted_id, 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]), @@ -160,14 +175,15 @@ module OpenProject::TextFormatting # WP label resolution is unscoped (preloaded above); visibility is # gated separately in `text_only?` so an inaccessible WP renders # its current formatted_id rather than the envelope text the - # author originally typed. User/group mentions stay visibility-gated - # at the find — there's no equivalent text-only render path for - # principals. + # author originally typed. Principals stay visibility-gated at the + # preload — there's no equivalent text-only render path for users + # or groups, so invisible principals fall back to the literal text. def class_from_mention(mention) + id = mention_id(mention)&.to_i case mention.attributes["data-type"].value - when "user" then User.visible.find_by(id: mention_id(mention)) - when "group" then Group.visible.find_by(id: mention_id(mention)) - when "work_package" then @mentioned_work_packages[mention_id(mention)&.to_i] + 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/helpers/static_macro_label.rb b/lib/open_project/text_formatting/helpers/static_macro_label.rb new file mode 100644 index 00000000000..b67b8b0fdce --- /dev/null +++ b/lib/open_project/text_formatting/helpers/static_macro_label.rb @@ -0,0 +1,53 @@ +# 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 + # Composes the anchor text for a work-package quickinfo macro rendered + # in a static-HTML channel (mailers, server-side previews) — channels + # that cannot hydrate the JS-driven `` widget + # and so flatten the macro to an `` whose text must carry enough + # context for a reader to recognise the reference: type, optional + # status, the identifier label, and the subject. + # + # Shared between the text-reference path (`LinkHandlers::WorkPackages`) + # and the envelope path (`Filters::MentionFilter`) so both render the + # same shape for the same WP. + 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 42e8ab769b1..60039848feb 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 @@ -38,14 +38,6 @@ module OpenProject::TextFormatting::Matchers # Shared with the PDF-export subclass in `app/models/work_package/exports/macros/links.rb`. HASH_TRIGGERS = %w[# ## ###].freeze - def self.compose_static_macro_label(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 - def applicable? hash_trigger? && matcher.prefix.nil? end @@ -109,7 +101,7 @@ module OpenProject::TextFormatting::Matchers 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) + label = WorkPackage::SemanticIdentifier.with_hash_prefix(display_id) return label if text_only?(work_package) return render_static_work_package_macro(work_package, label, detailed:) if context[:as_static_html] @@ -129,7 +121,7 @@ module OpenProject::TextFormatting::Matchers def render_static_work_package_macro(work_package, label, detailed:) return label unless work_package - link_to(self.class.compose_static_macro_label(work_package, label:, detailed:), + 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 @@ -155,7 +147,9 @@ module OpenProject::TextFormatting::Matchers # Plain-text channels and inaccessible WPs both render the label # without an anchor or quickinfo. Visibility is checked only when a # WP was preloaded — a nil work_package means a classic-mode render - # or an unresolved reference, neither of which needs gating. + # or an unresolved reference, neither of which needs gating. Mirrors + # `MentionFilter#text_only?`, which gates the same decision for + # `` envelopes; keep the two in sync. def text_only?(work_package) context[:as_text] || (work_package && !preload_cache.visible?(work_package.id)) 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 c7a420ffa77..f2bd67083d8 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -90,6 +90,8 @@ module OpenProject::TextFormatting visible_ids.include?(work_package_id) end + # Per-process frozen singleton returned when no preload is in + # scope — not a factory; callers must not mutate it. EMPTY = new(lookup: {}.freeze, visible_ids: Set.new.freeze).freeze end @@ -164,10 +166,11 @@ module OpenProject::TextFormatting # Doc-level preload called by `PatternMatcherFilter`. Save/restores # the cache so a nested `format_text` (e.g. custom-field formatter # re-entering the pipeline) doesn't clobber the outer render. - # Classic mode normally skips the load (the link handler renders - # `#N` from the matched id alone), but static-HTML channels need - # the WP record in both modes to compose the type/subject/status - # anchor. + # Fires when semantic identifiers need resolving or when a + # static-HTML channel needs the WP record to compose the + # type/subject/status anchor; other channels render `#N` from the + # matched id alone and the as-text path short-circuits on + # `text_only?` before consulting the cache. def self.with_preloaded_resources(doc, context) previous = RequestStore.store[CACHE_KEY] return yield unless Setting::WorkPackageIdentifier.semantic? || context[:as_static_html] diff --git a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb index a0b6268516d..6151b52dc57 100644 --- a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb +++ b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb @@ -225,6 +225,29 @@ RSpec.describe OpenProject::TextFormatting::Filters::MentionFilter do end end + context "in plain-text rendering mode (classic)", + with_flag: { semantic_work_package_ids: false }, + with_settings: { work_packages_identifier: "classic" } do + let(:project) { create(:project, identifier: "macroproj") } + let(:work_package) { create(:work_package, project:, author:) } + + it "renders the hash-prefixed numeric id without an anchor or quickinfo" do + rendered = format_text(mention_tag(work_package), format: :markdown_as_text) + + expect(rendered).to include("##{work_package.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_formatter_spec.rb b/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb index 4dc9232ad71..f42c7bbdbb3 100644 --- a/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb +++ b/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb @@ -145,6 +145,23 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt expect(rendered).not_to match(%r{]*>[^<]*SECRET-1}) end end + + context "in classic mode", + with_flag: { semantic_work_package_ids: false }, + with_settings: { work_packages_identifier: "classic" } do + it "renders the bare #N label without an anchor for ##N" do + rendered = described_class.new(context).to_html("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 = described_class.new(context).to_html("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 @@ -165,6 +182,48 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt 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_flag: { semantic_work_package_ids: true }, + 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 { described_class.new(context).to_html("see #{'##'}#{private_wp.id}") } + .not_to raise_error + + rendered = described_class.new(context).to_html("see #{'##'}#{private_wp.id}") + expect(rendered).to include("PRIVATE-1") + end + end + + context "in classic mode", + with_flag: { semantic_work_package_ids: false }, + with_settings: { work_packages_identifier: "classic" } do + it "does not raise and renders the #N text" do + expect { described_class.new(context).to_html("see #{'##'}#{private_wp.id}") } + .not_to raise_error + + rendered = described_class.new(context).to_html("see #{'##'}#{private_wp.id}") + expect(rendered).to include("##{private_wp.id}") + end + end + end + describe "format identifier" do it "exposes :markdown_as_static_html" do expect(described_class.format).to eq(:markdown_as_static_html) diff --git a/spec/lib/open_project/text_formatting/formats/markdown/text_formatter_spec.rb b/spec/lib/open_project/text_formatting/formats/markdown/text_formatter_spec.rb index 501f3d997f4..25b408dae2f 100644 --- a/spec/lib/open_project/text_formatting/formats/markdown/text_formatter_spec.rb +++ b/spec/lib/open_project/text_formatting/formats/markdown/text_formatter_spec.rb @@ -97,6 +97,20 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::TextFormatter do expect(described_class.new({}).to_html(input).strip).to eq("see DEMO-1 please") end end + + context "in classic mode", + with_flag: { semantic_work_package_ids: false }, + with_settings: { work_packages_identifier: "classic" } do + it "renders ##N as the hash-prefixed numeric id" do + input = "see #{'##'}#{work_package.id} please" + expect(described_class.new({}).to_html(input).strip).to eq("see ##{work_package.id} please") + end + + it "renders ###N as the hash-prefixed numeric id" do + input = "see #{'###'}#{work_package.id} please" + expect(described_class.new({}).to_html(input).strip).to eq("see ##{work_package.id} please") + end + end end describe "with a work-package mention envelope" do From 989dbf9da8b282013a54147545027cb145640093 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 14:22:25 +0300 Subject: [PATCH 12/20] Name the preload-required predicate Compress the "do we need to load WP records?" condition into `preload_required?(context)` so the call site reads as intent rather than a tangle of two unrelated signals. The reasoning (semantic mode needs row lookup; static-HTML output needs type/subject for the anchor) moves to a comment on the predicate, where it belongs. --- .../matchers/resource_links_matcher.rb | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) 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 f2bd67083d8..772b98a4333 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -166,14 +166,9 @@ module OpenProject::TextFormatting # Doc-level preload called by `PatternMatcherFilter`. Save/restores # the cache so a nested `format_text` (e.g. custom-field formatter # re-entering the pipeline) doesn't clobber the outer render. - # Fires when semantic identifiers need resolving or when a - # static-HTML channel needs the WP record to compose the - # type/subject/status anchor; other channels render `#N` from the - # matched id alone and the as-text path short-circuits on - # `text_only?` before consulting the cache. def self.with_preloaded_resources(doc, context) previous = RequestStore.store[CACHE_KEY] - return yield unless Setting::WorkPackageIdentifier.semantic? || context[:as_static_html] + return yield unless preload_required?(context) identifiers = collect_work_package_identifiers(doc) return yield if identifiers.empty? @@ -184,6 +179,17 @@ module OpenProject::TextFormatting RequestStore.store[CACHE_KEY] = previous end + # Two channels need the WP record at render time: semantic mode (to + # resolve `PROJ-7` to a row) and static-HTML output (to compose the + # type/subject anchor of a quickinfo macro). Classic-mode rich HTML + # and the as-text channel both render from the matched id alone — + # the latter short-circuits on `text_only?` before consulting the + # cache. + def self.preload_required?(context) + Setting::WorkPackageIdentifier.semantic? || context[:as_static_html] + end + private_class_method :preload_required? + def self.collect_work_package_identifiers(doc) identifiers = Set.new doc.search(".//text()").each do |node| From 23d52fcf1c3690dc5a463d8dc7317b2f08585c22 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 14:22:33 +0300 Subject: [PATCH 13/20] Drop semantic_work_package_ids flag annotations The feature flag is gone on release/17.5 (PR #23324); the `with_settings: { work_packages_identifier: ... }` annotation alone is enough to pin classic vs semantic behaviour in each context. --- .../text_formatting/filters/mention_filter_spec.rb | 3 --- .../formats/markdown/static_html_formatter_spec.rb | 10 ---------- .../formats/markdown/text_formatter_spec.rb | 7 ------- spec/mailers/work_package_mailer_spec.rb | 7 ------- .../work_packages/update_ancestors_service_spec.rb | 4 +--- 5 files changed, 1 insertion(+), 30 deletions(-) diff --git a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb index 6151b52dc57..4e87eb77eec 100644 --- a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb +++ b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb @@ -172,7 +172,6 @@ RSpec.describe OpenProject::TextFormatting::Filters::MentionFilter do end context "with a mention to an inaccessible WP", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do # Label resolution is unscoped so the envelope renders the WP's # current `formatted_id` (e.g. `HIDDEN-1`) rather than the literal @@ -204,7 +203,6 @@ RSpec.describe OpenProject::TextFormatting::Filters::MentionFilter do end context "in plain-text rendering mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do # The `:markdown_as_text` channel must collapse mention envelopes # to their current `formatted_id` so the plain-text mailer doesn't @@ -226,7 +224,6 @@ RSpec.describe OpenProject::TextFormatting::Filters::MentionFilter do end context "in plain-text rendering mode (classic)", - with_flag: { semantic_work_package_ids: false }, with_settings: { work_packages_identifier: "classic" } do let(:project) { create(:project, identifier: "macroproj") } let(:work_package) { create(:work_package, project:, author:) } diff --git a/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb b/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb index f42c7bbdbb3..54ed6c2dc27 100644 --- a/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb +++ b/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb @@ -57,7 +57,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt let(:input) { "see ##{work_package.id} please" } context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before { work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) } @@ -72,7 +71,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt let(:input) { "see #{'##'}#{work_package.id}" } context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before { work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) } @@ -87,7 +85,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt end context "in classic mode", - with_flag: { semantic_work_package_ids: false }, 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}) @@ -99,7 +96,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt let(:input) { "see #{'###'}#{work_package.id}" } context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before { work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) } @@ -109,7 +105,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt end context "in classic mode", - with_flag: { semantic_work_package_ids: false }, 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}) @@ -128,7 +123,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt before { login_as(reader) } context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before { hidden_wp.update_columns(identifier: "SECRET-1", sequence_number: 1) } @@ -147,7 +141,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt end context "in classic mode", - with_flag: { semantic_work_package_ids: false }, with_settings: { work_packages_identifier: "classic" } do it "renders the bare #N label without an anchor for ##N" do rendered = described_class.new(context).to_html("see #{'##'}#{hidden_wp.id}") @@ -171,7 +164,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt let(:input) { %(check ##DEMO-1) } context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before { work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) } @@ -198,7 +190,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt end context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before { private_wp.update_columns(identifier: "PRIVATE-1", sequence_number: 1) } @@ -212,7 +203,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt end context "in classic mode", - with_flag: { semantic_work_package_ids: false }, with_settings: { work_packages_identifier: "classic" } do it "does not raise and renders the #N text" do expect { described_class.new(context).to_html("see #{'##'}#{private_wp.id}") } diff --git a/spec/lib/open_project/text_formatting/formats/markdown/text_formatter_spec.rb b/spec/lib/open_project/text_formatting/formats/markdown/text_formatter_spec.rb index 25b408dae2f..44925d7ce4d 100644 --- a/spec/lib/open_project/text_formatting/formats/markdown/text_formatter_spec.rb +++ b/spec/lib/open_project/text_formatting/formats/markdown/text_formatter_spec.rb @@ -53,7 +53,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::TextFormatter do before { login_as(admin) } context "in classic mode", - with_flag: { semantic_work_package_ids: false }, with_settings: { work_packages_identifier: "classic" } do it "renders the hash-prefixed numeric id" do expect(formatted).to eq("see ##{work_package.id} please") @@ -61,7 +60,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::TextFormatter do end context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before do work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) @@ -81,7 +79,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::TextFormatter do before { login_as(admin) } context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before do work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) @@ -99,7 +96,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::TextFormatter do end context "in classic mode", - with_flag: { semantic_work_package_ids: false }, with_settings: { work_packages_identifier: "classic" } do it "renders ##N as the hash-prefixed numeric id" do input = "see #{'##'}#{work_package.id} please" @@ -126,7 +122,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::TextFormatter do before { login_as(admin) } context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before do work_package.update_columns(identifier: "DEMO-1", sequence_number: 1) @@ -138,7 +133,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::TextFormatter do end context "in classic mode", - with_flag: { semantic_work_package_ids: false }, with_settings: { work_packages_identifier: "classic" } do it "unwraps to the hash-prefixed numeric id" do expect(formatted).to eq("check ##{work_package.id}") @@ -146,7 +140,6 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::TextFormatter do end context "with a ##-shaped mention text in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do let(:input) { %(check ##DEMO-1) } diff --git a/spec/mailers/work_package_mailer_spec.rb b/spec/mailers/work_package_mailer_spec.rb index 017aa2811cb..93fab35f2f2 100644 --- a/spec/mailers/work_package_mailer_spec.rb +++ b/spec/mailers/work_package_mailer_spec.rb @@ -146,7 +146,6 @@ RSpec.describe WorkPackageMailer do let(:mail) { described_class.mentioned(persisted_recipient, persisted_journal) } context "with classic mode", - with_flag: { semantic_work_package_ids: false }, 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}") @@ -154,7 +153,6 @@ RSpec.describe WorkPackageMailer do end context "with semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before do referenced_wp.update_columns(identifier: "DEMO-1", sequence_number: 1) @@ -264,7 +262,6 @@ RSpec.describe WorkPackageMailer do end context "with classic mode", - with_flag: { semantic_work_package_ids: false }, 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}") @@ -272,7 +269,6 @@ RSpec.describe WorkPackageMailer do end context "with semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before do referenced_wp.update_columns(identifier: "DEMO-1", sequence_number: 1) @@ -292,7 +288,6 @@ RSpec.describe WorkPackageMailer do end describe "rendering a cross-project WP reference to a recipient without visibility", - with_flag: { semantic_work_package_ids: true }, 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") } @@ -349,7 +344,6 @@ RSpec.describe WorkPackageMailer do end context "with semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_packages_identifier: "semantic" } do before { referenced_wp.update_columns(identifier: "DEMO-1", sequence_number: 1) } @@ -369,7 +363,6 @@ RSpec.describe WorkPackageMailer do end context "with classic mode", - with_flag: { semantic_work_package_ids: false }, 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 diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index baf6321e6fd..3a9ad8fc105 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -1397,9 +1397,8 @@ RSpec.describe WorkPackages::UpdateAncestorsService, # 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 identifier renames and feature-flag rollback. + # text survives project-identifier renames. context "in classic mode", - with_flag: { semantic_work_package_ids: false }, 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) @@ -1411,7 +1410,6 @@ RSpec.describe WorkPackages::UpdateAncestorsService, end context "in semantic mode", - with_flag: { semantic_work_package_ids: true }, with_settings: { work_package_done_ratio: "status", work_packages_identifier: "semantic" } do before do child.allocate_and_register_semantic_id From 2ca379fe375c1e1ec7a136e580a768ee3310bc70 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 14:32:37 +0300 Subject: [PATCH 14/20] Trim verbose comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strip explanatory paragraphs and cross-reference jargon ("Mirrors X", "Shared with Y", etc.) from comments introduced on this branch. Keep only the WHY — the parts of the design intent that aren't already evident from method names and short bodies. Pre-existing comments on methods this branch didn't author stay as-is. --- .../text_formatting/filters/mention_filter.rb | 37 ++++---------- .../filters/plain_text_output_filter.rb | 6 +-- .../formats/markdown/static_html_formatter.rb | 11 ++--- .../formats/markdown/text_formatter.rb | 8 ++-- .../helpers/static_macro_label.rb | 13 ++--- .../matchers/link_handlers/work_packages.rb | 17 ++----- .../matchers/resource_links_matcher.rb | 48 +++++-------------- 7 files changed, 38 insertions(+), 102 deletions(-) diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 9ddef4e1285..2b85decfc00 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -49,13 +49,11 @@ module OpenProject::TextFormatting private - # Doc-level preload so a note with N user/group/WP mentions costs - # a bounded number of SELECTs rather than one per mention. WP - # mentions need the unscoped record (label) plus a separate - # visibility id pluck (anchor-vs-text gating); principals fold the - # two concerns into a single visibility-scoped fetch since - # invisible principals have no plain-text render path — they fall - # back to the literal envelope text instead. + # 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 @@ -70,8 +68,6 @@ module OpenProject::TextFormatting end scope = WorkPackage.where(id: ids) - # Static-HTML channels need `type` and `status` to render - # quickinfo envelopes as anchors instead of `` widgets. scope = scope.includes(:type, :status) if context[:as_static_html] @mentioned_work_packages = scope.index_by(&:id) @visible_mentioned_ids = WorkPackage.visible.where(id: ids).pluck(:id).to_set @@ -118,8 +114,6 @@ module OpenProject::TextFormatting def work_package_mention(work_package, mention) return Nokogiri::XML::Text.new(work_package.formatted_id, mention.document) if text_only?(work_package) - # Match the label and URL convention used for `#N` text references - # elsewhere in the markdown pipeline. case mention.text.count("#") when 3 then work_package_quickinfo(work_package, detailed: true) when 2 then work_package_quickinfo(work_package, detailed: false) @@ -127,12 +121,8 @@ module OpenProject::TextFormatting end end - # Plain-text channels and inaccessible WPs both render the - # `formatted_id` without an anchor or quickinfo widget — the - # latter would resolve to a hover-card endpoint the recipient - # can't reach. Mirrors `LinkHandlers::WorkPackages#text_only?`, - # which gates the matching decision for `#N` text references; keep - # the two in sync. + # 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[:as_text] || @visible_mentioned_ids.exclude?(work_package.id) end @@ -147,11 +137,8 @@ module OpenProject::TextFormatting detailed: } end - # Static fallback shared with the PatternMatcherFilter's `##`/`###` - # path so envelope-driven and text-driven references render the same - # shape in channels that cannot hydrate the custom element. Always - # uses the WP's current `formatted_id`, normalising any historical - # alias the envelope may have been authored with. + # 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:) @@ -172,12 +159,6 @@ module OpenProject::TextFormatting }) end - # WP label resolution is unscoped (preloaded above); visibility is - # gated separately in `text_only?` so an inaccessible WP renders - # its current formatted_id rather than the envelope text the - # author originally typed. Principals stay visibility-gated at the - # preload — there's no equivalent text-only render path for users - # or groups, so invisible principals fall back to the literal text. def class_from_mention(mention) id = mention_id(mention)&.to_i case mention.attributes["data-type"].value 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 index b23c992af0e..32873ddcb20 100644 --- a/lib/open_project/text_formatting/filters/plain_text_output_filter.rb +++ b/lib/open_project/text_formatting/filters/plain_text_output_filter.rb @@ -30,10 +30,8 @@ module OpenProject::TextFormatting module Filters - # Final stage of the plain-text pipeline. Earlier filters resolve - # mentions and macros to their text-mode shapes (driven by - # `context[:as_text]`); this stage collapses any remaining markup - # so the pipeline output is suitable for `text/plain` bodies. + # Final stage of the `markdown_as_text` pipeline — strips remaining + # markup so the output is safe for `text/plain` bodies. class PlainTextOutputFilter < HTML::Pipeline::Filter def call doc.text diff --git a/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb b/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb index dcf25ca8b5a..0cca7df54f8 100644 --- a/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb +++ b/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb @@ -30,13 +30,10 @@ module OpenProject::TextFormatting::Formats module Markdown - # Static-HTML sibling of `Markdown::Formatter`. Shares the same filter - # chain so identifier resolution, mention handling, and link rendering - # stay consistent, but signals `context[:as_static_html]` so matchers - # and filters emit server-rendered anchors in place of JS-hydrated - # custom elements. Intended for channels that cannot run JS — HTML - # mailers, server-side previews, archival exports — where dynamic - # widgets would collapse to empty placeholders. + # Inherits `Markdown::Formatter`'s filter chain but signals + # `context[:as_static_html]` so matchers emit server-rendered + # anchors instead of JS-hydrated custom elements. For channels + # that can't run JS — HTML mailers, server-side previews. class StaticHtmlFormatter < Formatter def initialize(context) super(context.merge(as_static_html: true)) diff --git a/lib/open_project/text_formatting/formats/markdown/text_formatter.rb b/lib/open_project/text_formatting/formats/markdown/text_formatter.rb index b320fb967e3..4f0430ed630 100644 --- a/lib/open_project/text_formatting/formats/markdown/text_formatter.rb +++ b/lib/open_project/text_formatting/formats/markdown/text_formatter.rb @@ -30,11 +30,9 @@ module OpenProject::TextFormatting::Formats module Markdown - # Text-output sibling of `Markdown::Formatter`. Shares the matcher and - # mention pipeline with the rich renderer so identifier resolution - # stays consistent across channels, then collapses the final DOM to - # text via `PlainTextOutputFilter`. Intended for plain/text mailers - # and other channels where HTML would be a foreign body. + # Runs the matcher and mention pipeline, then collapses the DOM to + # text via `PlainTextOutputFilter`. For `text/plain` mailers and + # other channels where HTML would be a foreign body. class TextFormatter < OpenProject::TextFormatting::Formats::BaseFormatter def initialize(context) super(context.merge(as_text: true)) diff --git a/lib/open_project/text_formatting/helpers/static_macro_label.rb b/lib/open_project/text_formatting/helpers/static_macro_label.rb index b67b8b0fdce..2c33e269044 100644 --- a/lib/open_project/text_formatting/helpers/static_macro_label.rb +++ b/lib/open_project/text_formatting/helpers/static_macro_label.rb @@ -30,16 +30,9 @@ module OpenProject::TextFormatting module Helpers - # Composes the anchor text for a work-package quickinfo macro rendered - # in a static-HTML channel (mailers, server-side previews) — channels - # that cannot hydrate the JS-driven `` widget - # and so flatten the macro to an `` whose text must carry enough - # context for a reader to recognise the reference: type, optional - # status, the identifier label, and the subject. - # - # Shared between the text-reference path (`LinkHandlers::WorkPackages`) - # and the envelope path (`Filters::MentionFilter`) so both render the - # same shape for the same WP. + # 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 = [] 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 60039848feb..c6c27c2369d 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 @@ -111,13 +111,8 @@ module OpenProject::TextFormatting::Matchers data: { id:, display_id:, detailed: } end - # Static fallback for channels that cannot hydrate the quickinfo - # custom element. Composes type, optional status, label, and subject - # into the anchor; unresolved references collapse to the bare label. - # The label is the matched identifier (potentially a historical alias) - # to preserve what the author wrote — the `` envelope path - # in `MentionFilter` instead normalises to the WP's current - # `formatted_id`. + # 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 @@ -144,12 +139,8 @@ module OpenProject::TextFormatting::Matchers }) end - # Plain-text channels and inaccessible WPs both render the label - # without an anchor or quickinfo. Visibility is checked only when a - # WP was preloaded — a nil work_package means a classic-mode render - # or an unresolved reference, neither of which needs gating. Mirrors - # `MentionFilter#text_only?`, which gates the same decision for - # `` envelopes; keep the two in sync. + # 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[:as_text] || (work_package && !preload_cache.visible?(work_package.id)) 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 772b98a4333..ae82e8669c6 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -69,11 +69,9 @@ module OpenProject::TextFormatting # identifier:version:1.0.0 # identifier:source:some/file class ResourceLinksMatcher < RegexMatcher - # Per-render preload state shared between the matcher and the link - # handler. Pairs unscoped label resolution (`lookup`) with viewer-scoped - # link gating (`visible_ids`) so a single render computes both with a - # bounded number of round-trips and renders consistent labels for - # invisible WPs. + # 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 @@ -90,8 +88,7 @@ module OpenProject::TextFormatting visible_ids.include?(work_package_id) end - # Per-process frozen singleton returned when no preload is in - # scope — not a factory; callers must not mutate it. + # Frozen singleton, not a factory — callers must not mutate it. EMPTY = new(lookup: {}.freeze, visible_ids: Set.new.freeze).freeze end @@ -156,16 +153,13 @@ module OpenProject::TextFormatting ] end - # The active preload cache for the current render, or an empty - # singleton when no preload is in scope (classic mode, no `#N` - # references, or rendering outside `with_preloaded_resources`). def self.current_cache RequestStore.store[CACHE_KEY] || WorkPackagePreloadCache::EMPTY end - # Doc-level preload called by `PatternMatcherFilter`. Save/restores - # the cache so a nested `format_text` (e.g. custom-field formatter - # re-entering the pipeline) doesn't clobber the outer render. + # 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) @@ -179,12 +173,8 @@ module OpenProject::TextFormatting RequestStore.store[CACHE_KEY] = previous end - # Two channels need the WP record at render time: semantic mode (to - # resolve `PROJ-7` to a row) and static-HTML output (to compose the - # type/subject anchor of a quickinfo macro). Classic-mode rich HTML - # and the as-text channel both render from the matched id alone — - # the latter short-circuits on `text_only?` before consulting the - # cache. + # 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[:as_static_html] end @@ -215,22 +205,10 @@ module OpenProject::TextFormatting identifier end - # Label resolution is unscoped: a reference to an inaccessible - # work package still renders as its semantic identifier (e.g. - # `DCP-1`), just without a navigable anchor. Visibility is - # captured separately so the link handler can pick anchor-vs-text - # per WP without re-querying. - # - # Two SELECTs in the common case: one unscoped fetch by identifier, - # one visibility-scoped id pluck. A third targeted SELECT fires - # for historical aliases — the loaded row carries only the current - # identifier, so unmapped inputs are filled in from - # `WorkPackageSemanticAlias`. Static-HTML channels also eager-load - # `:type` and `:status` so the link handler can render the - # static-anchor variant of `##`/`###` macros without N+1 queries — - # those associations are the metadata a reader needs to recognise a - # WP reference flattened to text. Anything beyond that (project, - # versions, custom fields) stays out of this preload. + # 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[:as_static_html] From 94c13c11fa8a0d2e09075fe3c08f501031080c28 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 14:50:59 +0300 Subject: [PATCH 15/20] Collapse static-HTML formatter into a context option The static-HTML pipeline differs from the rich pipeline only by a context flag - both share the same filter chain. The dedicated `Markdown::StaticHtmlFormatter` and `:markdown_as_static_html` format symbol were pure boilerplate around that one-line override. Callers now pass `format: :rich, static_html: true` and the matchers read `context[:static_html]` directly. --- .../_work_package_details.html.erb | 2 +- .../work_package_mailer/mentioned.html.erb | 2 +- .../watcher_changed.html.erb | 3 +- .../text_formatting/filters/mention_filter.rb | 4 +- .../formats/markdown/static_html_formatter.rb | 47 ------------------- .../matchers/link_handlers/work_packages.rb | 2 +- .../matchers/resource_links_matcher.rb | 4 +- lib/open_project/text_formatting/renderer.rb | 4 +- ..._spec.rb => static_html_rendering_spec.rb} | 41 +++++++--------- 9 files changed, 28 insertions(+), 81 deletions(-) delete mode 100644 lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb rename spec/lib/open_project/text_formatting/formats/markdown/{static_html_formatter_spec.rb => static_html_rendering_spec.rb} (86%) 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 5f53d3e9ef9..d9f541fd1a1 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, format: :markdown_as_static_html, only_path: false) %> +<%= format_text(work_package, :description, format: :rich, static_html: true, only_path: false) %> diff --git a/app/views/work_package_mailer/mentioned.html.erb b/app/views/work_package_mailer/mentioned.html.erb index eda3d8e4fb2..204fca23e38 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, format: :markdown_as_static_html, only_path: false %> + <%= format_text @journal.notes, format: :rich, static_html: true, only_path: false %>
diff --git a/app/views/work_package_mailer/watcher_changed.html.erb b/app/views/work_package_mailer/watcher_changed.html.erb index 2275780822e..b99267338ac 100644 --- a/app/views/work_package_mailer/watcher_changed.html.erb +++ b/app/views/work_package_mailer/watcher_changed.html.erb @@ -32,7 +32,8 @@ See COPYRIGHT and LICENSE files for more details.

<%= format_text( t(:text_latest_note, note: last_work_package_note(@work_package)), - format: :markdown_as_static_html, + format: :rich, + static_html: true, only_path: false, object: @work_package, project: @work_package.project diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 2b85decfc00..51a0deb433b 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -68,7 +68,7 @@ module OpenProject::TextFormatting end scope = WorkPackage.where(id: ids) - scope = scope.includes(:type, :status) if context[:as_static_html] + 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 @@ -128,7 +128,7 @@ module OpenProject::TextFormatting end def work_package_quickinfo(work_package, detailed:) - return work_package_static_macro(work_package, detailed:) if context[:as_static_html] + return work_package_static_macro(work_package, detailed:) if context[:static_html] ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", diff --git a/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb b/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb deleted file mode 100644 index 0cca7df54f8..00000000000 --- a/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb +++ /dev/null @@ -1,47 +0,0 @@ -# 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::Formats - module Markdown - # Inherits `Markdown::Formatter`'s filter chain but signals - # `context[:as_static_html]` so matchers emit server-rendered - # anchors instead of JS-hydrated custom elements. For channels - # that can't run JS — HTML mailers, server-side previews. - class StaticHtmlFormatter < Formatter - def initialize(context) - super(context.merge(as_static_html: true)) - end - - def self.format - :markdown_as_static_html - 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 c6c27c2369d..99a275fef29 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 @@ -104,7 +104,7 @@ module OpenProject::TextFormatting::Matchers label = WorkPackage::SemanticIdentifier.with_hash_prefix(display_id) return label if text_only?(work_package) - return render_static_work_package_macro(work_package, label, detailed:) if context[:as_static_html] + return render_static_work_package_macro(work_package, label, detailed:) if context[:static_html] ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", 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 ae82e8669c6..667fb9afe19 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -176,7 +176,7 @@ module OpenProject::TextFormatting # 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[:as_static_html] + Setting::WorkPackageIdentifier.semantic? || context[:static_html] end private_class_method :preload_required? @@ -211,7 +211,7 @@ module OpenProject::TextFormatting # the anchor for `##`/`###` macros. def self.build_cache(identifiers, context = {}) scope = WorkPackage.where_display_id_in(*identifiers) - scope = if context[:as_static_html] + scope = if context[:static_html] scope.includes(:type, :status) else scope.select(:id, :identifier) diff --git a/lib/open_project/text_formatting/renderer.rb b/lib/open_project/text_formatting/renderer.rb index 68b9e6187ad..d8f29bfd70e 100644 --- a/lib/open_project/text_formatting/renderer.rb +++ b/lib/open_project/text_formatting/renderer.rb @@ -49,7 +49,7 @@ module OpenProject::TextFormatting .to_html(text) end - # @param [:plain, :markdown_as_text, :markdown_as_static_html, :rich] format the text format. + # @param [:plain, :markdown_as_text, :rich] format the text format. # @return [Formats::BaseFormatter] a formatter implementation. def formatter_for(format) case format.to_sym @@ -57,8 +57,6 @@ module OpenProject::TextFormatting Formats.plain_formatter when :markdown_as_text Formats::Markdown::TextFormatter - when :markdown_as_static_html - Formats::Markdown::StaticHtmlFormatter else Formats.rich_formatter end diff --git a/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb b/spec/lib/open_project/text_formatting/formats/markdown/static_html_rendering_spec.rb similarity index 86% rename from spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb rename to spec/lib/open_project/text_formatting/formats/markdown/static_html_rendering_spec.rb index 54ed6c2dc27..f17f3ce4407 100644 --- a/spec/lib/open_project/text_formatting/formats/markdown/static_html_formatter_spec.rb +++ b/spec/lib/open_project/text_formatting/formats/markdown/static_html_rendering_spec.rb @@ -30,8 +30,16 @@ require "spec_helper" -RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatter do - subject(:formatted) { described_class.new(context).to_html(input) } +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 } } @@ -127,14 +135,14 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt before { hidden_wp.update_columns(identifier: "SECRET-1", sequence_number: 1) } it "renders the bare identifier label without an anchor for ##N" do - rendered = described_class.new(context).to_html("see #{'##'}#{hidden_wp.id}") + 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 = described_class.new(context).to_html("see #{'###'}#{hidden_wp.id}") + rendered = render("see #{'###'}#{hidden_wp.id}") expect(rendered).to include("SECRET-1") expect(rendered).not_to match(%r{]*>[^<]*SECRET-1}) end @@ -143,14 +151,14 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt context "in classic mode", with_settings: { work_packages_identifier: "classic" } do it "renders the bare #N label without an anchor for ##N" do - rendered = described_class.new(context).to_html("see #{'##'}#{hidden_wp.id}") + 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 = described_class.new(context).to_html("see #{'###'}#{hidden_wp.id}") + rendered = render("see #{'###'}#{hidden_wp.id}") expect(rendered).to include("##{hidden_wp.id}") expect(rendered).not_to match(%r{]*>[^<]*##{hidden_wp.id}}) end @@ -194,10 +202,10 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt before { private_wp.update_columns(identifier: "PRIVATE-1", sequence_number: 1) } it "does not raise and renders the identifier text" do - expect { described_class.new(context).to_html("see #{'##'}#{private_wp.id}") } + expect { render("see #{'##'}#{private_wp.id}") } .not_to raise_error - rendered = described_class.new(context).to_html("see #{'##'}#{private_wp.id}") + rendered = render("see #{'##'}#{private_wp.id}") expect(rendered).to include("PRIVATE-1") end end @@ -205,25 +213,12 @@ RSpec.describe OpenProject::TextFormatting::Formats::Markdown::StaticHtmlFormatt context "in classic mode", with_settings: { work_packages_identifier: "classic" } do it "does not raise and renders the #N text" do - expect { described_class.new(context).to_html("see #{'##'}#{private_wp.id}") } + expect { render("see #{'##'}#{private_wp.id}") } .not_to raise_error - rendered = described_class.new(context).to_html("see #{'##'}#{private_wp.id}") + rendered = render("see #{'##'}#{private_wp.id}") expect(rendered).to include("##{private_wp.id}") end end end - - describe "format identifier" do - it "exposes :markdown_as_static_html" do - expect(described_class.format).to eq(:markdown_as_static_html) - end - end - - describe "renderer routing" do - it "Renderer.formatter_for(:markdown_as_static_html) resolves to this class" do - expect(OpenProject::TextFormatting::Renderer.formatter_for(:markdown_as_static_html)) - .to eq(described_class) - end - end end From 776536d88b08e37b70f669238a574b1a4484ea43 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 14:56:03 +0300 Subject: [PATCH 16/20] Drop redundant explicit format: :rich MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `:rich` is the default at every layer of the `format_text` chain (view helper → `OpenProject::TextFormatting#format_text` → `Renderer.format_text`), so the explicit keyword adds noise. --- app/views/work_package_mailer/_work_package_details.html.erb | 2 +- app/views/work_package_mailer/mentioned.html.erb | 2 +- app/views/work_package_mailer/watcher_changed.html.erb | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) 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 d9f541fd1a1..d6f2820927d 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, format: :rich, static_html: true, only_path: false) %> +<%= format_text(work_package, :description, static_html: true, only_path: false) %> diff --git a/app/views/work_package_mailer/mentioned.html.erb b/app/views/work_package_mailer/mentioned.html.erb index 204fca23e38..b498dff23bc 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, format: :rich, static_html: true, only_path: false %> + <%= format_text @journal.notes, static_html: true, only_path: false %>
diff --git a/app/views/work_package_mailer/watcher_changed.html.erb b/app/views/work_package_mailer/watcher_changed.html.erb index b99267338ac..34bc9b746ae 100644 --- a/app/views/work_package_mailer/watcher_changed.html.erb +++ b/app/views/work_package_mailer/watcher_changed.html.erb @@ -32,7 +32,6 @@ See COPYRIGHT and LICENSE files for more details.

<%= format_text( t(:text_latest_note, note: last_work_package_note(@work_package)), - format: :rich, static_html: true, only_path: false, object: @work_package, From c607b36b8e5cdf553087009bc8b80fe61d697fba Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 19:40:47 +0300 Subject: [PATCH 17/20] Rename with_hash_prefix to format_display_id --- app/models/work_package/semantic_identifier.rb | 6 +++--- .../matchers/link_handlers/work_packages.rb | 2 +- spec/models/work_package/semantic_identifier_spec.rb | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index b8b728a9069..43c9769d59e 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -124,8 +124,8 @@ module WorkPackage::SemanticIdentifier # Returns formatted value for inline UI display. # * Semantic mode: "PROJ-42" (no prefix — self-describing) # * Classic mode: "#42" (hash-prefixed) - def self.with_hash_prefix(value) - value.is_a?(String) && value.match?(/[A-Za-z]/) ? value : "##{value}" + 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. @@ -141,7 +141,7 @@ module WorkPackage::SemanticIdentifier # Semantic mode: "PROJ-42" (no prefix — self-describing) # Classic mode: "#42" (hash-prefixed) def formatted_id - WorkPackage::SemanticIdentifier.with_hash_prefix(display_id) + WorkPackage::SemanticIdentifier.format_display_id(display_id) end # Override ActiveRecord's default `to_param` so Rails URL helpers 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 99a275fef29..8d2c856720c 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 @@ -101,7 +101,7 @@ module OpenProject::TextFormatting::Matchers 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.with_hash_prefix(display_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] diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index cbd8a35731e..c07349d42c1 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -606,17 +606,17 @@ RSpec.describe WorkPackage::SemanticIdentifier do end end - describe ".format" do + describe ".format_display_id" do it "returns the semantic identifier unchanged when it carries letters" do - expect(described_class.format("MYPROJ-1")).to eq("MYPROJ-1") + expect(described_class.format_display_id("MYPROJ-1")).to eq("MYPROJ-1") end it "hash-prefixes a numeric integer" do - expect(described_class.format(42)).to eq("#42") + expect(described_class.format_display_id(42)).to eq("#42") end it "hash-prefixes a numeric string" do - expect(described_class.format("42")).to eq("#42") + expect(described_class.format_display_id("42")).to eq("#42") end end From f5957d800f8ad6e5e29c9e4dea360965e100ea3d Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 21:09:29 +0300 Subject: [PATCH 18/20] Collapse plain-text formatter into a context option Same pattern as the static-HTML collapse: the `markdown_as_text` format symbol was a thin subclass setting a context flag and swapping the filter list. Replace it with `plain_text: true` on the existing rich formatter, which now picks between `RICH_FILTERS` and `TEXT_FILTERS` constants based on the flag. `static_html:` and `plain_text:` now sit as peer options on one format. Rename the `as_text` context key to `plain_text` for symmetry with `static_html`. Update both mailer `.text.erb` views and the two handler predicates that branch on the flag. --- .../work_package_mailer/mentioned.text.erb | 2 +- .../watcher_changed.text.erb | 2 +- .../text_formatting/filters/mention_filter.rb | 2 +- .../filters/plain_text_output_filter.rb | 4 +- .../formats/markdown/formatter.rb | 56 +++++++++++------ .../formats/markdown/text_formatter.rb | 61 ------------------- .../matchers/link_handlers/work_packages.rb | 2 +- lib/open_project/text_formatting/renderer.rb | 4 +- .../filters/mention_filter_spec.rb | 10 +-- ...rmatter_spec.rb => text_rendering_spec.rb} | 20 +++--- 10 files changed, 57 insertions(+), 106 deletions(-) delete mode 100644 lib/open_project/text_formatting/formats/markdown/text_formatter.rb rename spec/lib/open_project/text_formatting/formats/markdown/{text_formatter_spec.rb => text_rendering_spec.rb} (85%) diff --git a/app/views/work_package_mailer/mentioned.text.erb b/app/views/work_package_mailer/mentioned.text.erb index 5b8ec48d8ca..e80e3fa8862 100644 --- a/app/views/work_package_mailer/mentioned.text.erb +++ b/app/views/work_package_mailer/mentioned.text.erb @@ -10,7 +10,7 @@ <%= I18n.t(:label_comment_added) %>: <%= format_text( @journal.notes, - format: :markdown_as_text, + plain_text: true, 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 8707c958f6c..b9e3a23d185 100644 --- a/app/views/work_package_mailer/watcher_changed.text.erb +++ b/app/views/work_package_mailer/watcher_changed.text.erb @@ -33,7 +33,7 @@ See COPYRIGHT and LICENSE files for more details. <%= render partial: "work_package_details", locals: { work_package: @work_package } %> <%= format_text( t(:text_latest_note, note: last_work_package_note(@work_package)), - format: :markdown_as_text, + plain_text: true, only_path: false, object: @work_package, project: @work_package.project diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 51a0deb433b..f1c8defddda 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -124,7 +124,7 @@ module OpenProject::TextFormatting # 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[:as_text] || @visible_mentioned_ids.exclude?(work_package.id) + context[:plain_text] || @visible_mentioned_ids.exclude?(work_package.id) end def work_package_quickinfo(work_package, detailed:) 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 index 32873ddcb20..34a2bbdada9 100644 --- a/lib/open_project/text_formatting/filters/plain_text_output_filter.rb +++ b/lib/open_project/text_formatting/filters/plain_text_output_filter.rb @@ -30,8 +30,8 @@ module OpenProject::TextFormatting module Filters - # Final stage of the `markdown_as_text` pipeline — strips remaining - # markup so the output is safe for `text/plain` bodies. + # 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 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/formats/markdown/text_formatter.rb b/lib/open_project/text_formatting/formats/markdown/text_formatter.rb deleted file mode 100644 index 4f0430ed630..00000000000 --- a/lib/open_project/text_formatting/formats/markdown/text_formatter.rb +++ /dev/null @@ -1,61 +0,0 @@ -# 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::Formats - module Markdown - # Runs the matcher and mention pipeline, then collapses the DOM to - # text via `PlainTextOutputFilter`. For `text/plain` mailers and - # other channels where HTML would be a foreign body. - class TextFormatter < OpenProject::TextFormatting::Formats::BaseFormatter - def initialize(context) - super(context.merge(as_text: true)) - end - - def to_html(text) - pipeline.call(text, context)[:output].to_s - end - - def 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 - ] - end - - def self.format - :markdown_as_text - 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 8d2c856720c..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 @@ -142,7 +142,7 @@ module OpenProject::TextFormatting::Matchers # 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[:as_text] || (work_package && !preload_cache.visible?(work_package.id)) + context[:plain_text] || (work_package && !preload_cache.visible?(work_package.id)) end def preload_cache diff --git a/lib/open_project/text_formatting/renderer.rb b/lib/open_project/text_formatting/renderer.rb index d8f29bfd70e..fd77761737e 100644 --- a/lib/open_project/text_formatting/renderer.rb +++ b/lib/open_project/text_formatting/renderer.rb @@ -49,14 +49,12 @@ module OpenProject::TextFormatting .to_html(text) end - # @param [:plain, :markdown_as_text, :rich] format the text format. + # @param [:plain, :rich] format the text format. # @return [Formats::BaseFormatter] a formatter implementation. def formatter_for(format) case format.to_sym when :plain Formats.plain_formatter - when :markdown_as_text - Formats::Markdown::TextFormatter else Formats.rich_formatter end diff --git a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb index 4e87eb77eec..0bf9b22a13f 100644 --- a/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb +++ b/spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb @@ -204,9 +204,9 @@ RSpec.describe OpenProject::TextFormatting::Filters::MentionFilter do context "in plain-text rendering mode", with_settings: { work_packages_identifier: "semantic" } do - # The `:markdown_as_text` channel must collapse mention envelopes - # to their current `formatted_id` so the plain-text mailer doesn't - # leak `` HTML or stale envelope text. + # `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:) } @@ -214,7 +214,7 @@ RSpec.describe OpenProject::TextFormatting::Filters::MentionFilter do it "renders the formatted_id without an anchor or quickinfo" do wp = work_package.reload - rendered = format_text(mention_tag(wp), format: :markdown_as_text) + rendered = format_text(mention_tag(wp), plain_text: true) expect(rendered).to include(wp.formatted_id) expect(rendered).not_to include(" Date: Wed, 27 May 2026 13:04:26 +0300 Subject: [PATCH 19/20] Add render_mode flag and MailFormattingHelper `format_text` accepts `render_mode:` (`:in_app_html`, `:external_html`, `:external_text`), which resolves the `only_path`, `static_html` and `plain_text` context flags as a set. External surfaces (mailer HTML body, future RSS/PDF/webhook) need absolute URLs and static rendering together; pinning the trio at the public API keeps callers from forgetting one. Explicit primitive kwargs still override. `MailFormattingHelper` exposes `format_mail_html` and `format_mail_text` thin wrappers around `format_text(render_mode:)`. The `_html` / `_text` suffix matches the `.html.erb` / `.text.erb` template extension so caller intent stays visible in the view, with no introspection of `formats`. The five WorkPackageMailer view sites use the helpers; `_work_package_details`, `mentioned.html`, `mentioned.text`, `watcher_changed.html`, `watcher_changed.text` drop the `static_html:`/`only_path:`/`plain_text:` boilerplate. --- app/helpers/mail_formatting_helper.rb | 44 +++++++++ app/mailers/application_mailer.rb | 1 + .../_work_package_details.html.erb | 2 +- .../work_package_mailer/mentioned.html.erb | 2 +- .../work_package_mailer/mentioned.text.erb | 4 +- .../watcher_changed.html.erb | 4 +- .../watcher_changed.text.erb | 4 +- lib/open_project/text_formatting.rb | 31 ++++-- .../text_formatting/render_mode.rb | 65 ++++++++++++ spec/helpers/mail_formatting_helper_spec.rb | 75 ++++++++++++++ .../text_formatting/render_mode_spec.rb | 98 +++++++++++++++++++ 11 files changed, 310 insertions(+), 20 deletions(-) create mode 100644 app/helpers/mail_formatting_helper.rb create mode 100644 lib/open_project/text_formatting/render_mode.rb create mode 100644 spec/helpers/mail_formatting_helper_spec.rb create mode 100644 spec/lib/open_project/text_formatting/render_mode_spec.rb diff --git a/app/helpers/mail_formatting_helper.rb b/app/helpers/mail_formatting_helper.rb new file mode 100644 index 00000000000..abaff32f244 --- /dev/null +++ b/app/helpers/mail_formatting_helper.rb @@ -0,0 +1,44 @@ +# 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. +#++ + +# Mailer-view wrappers around {OpenProject::TextFormatting#format_text}. They +# 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` template extension to the helper name +# keeps caller intent visible at the call site. +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/views/work_package_mailer/_work_package_details.html.erb b/app/views/work_package_mailer/_work_package_details.html.erb index d6f2820927d..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, static_html: true, 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 b498dff23bc..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, static_html: true, 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 e80e3fa8862..f93db4e5806 100644 --- a/app/views/work_package_mailer/mentioned.text.erb +++ b/app/views/work_package_mailer/mentioned.text.erb @@ -8,10 +8,8 @@ <%= "=" * ((@work_package.formatted_id + " " + @work_package.subject).length + 4) %> <%= I18n.t(:label_comment_added) %>: -<%= format_text( +<%= format_mail_text( @journal.notes, - plain_text: true, - only_path: false, object: @work_package, project: @work_package.project ) %> diff --git a/app/views/work_package_mailer/watcher_changed.html.erb b/app/views/work_package_mailer/watcher_changed.html.erb index 34bc9b746ae..bd169b9be43 100644 --- a/app/views/work_package_mailer/watcher_changed.html.erb +++ b/app/views/work_package_mailer/watcher_changed.html.erb @@ -30,10 +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)), - static_html: true, - 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 b9e3a23d185..841f65c0fe3 100644 --- a/app/views/work_package_mailer/watcher_changed.text.erb +++ b/app/views/work_package_mailer/watcher_changed.text.erb @@ -31,10 +31,8 @@ See COPYRIGHT and LICENSE files for more details. ---------------------------------------- <%= render partial: "work_package_details", locals: { work_package: @work_package } %> -<%= format_text( +<%= format_mail_text( t(:text_latest_note, note: last_work_package_note(@work_package)), - plain_text: true, - only_path: false, 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/render_mode.rb b/lib/open_project/text_formatting/render_mode.rb new file mode 100644 index 00000000000..3305c629b73 --- /dev/null +++ b/lib/open_project/text_formatting/render_mode.rb @@ -0,0 +1,65 @@ +# 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. + # + # The collapse exists because external surfaces (mailers today; RSS, PDF, + # webhooks tomorrow) always need absolute URLs *and* static rendering for + # JS-dependent components — the two flags never travel apart in practice. + # 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(" Date: Wed, 27 May 2026 13:22:56 +0300 Subject: [PATCH 20/20] Tighten render_mode and mail formatting helper docstrings Strip a forward-looking aside about future external surfaces in RenderMode; the invariant is that external surfaces need both absolute URLs and static rendering. Replace "in practice" with "a coupled set" to drop the soft hedge. Drop the lead "wrappers around format_text" sentence on MailFormattingHelper since the module body already shows the wrapping; the WHY (channel pinning, extension/helper name parity) is the part worth documenting. --- app/helpers/mail_formatting_helper.rb | 9 ++++----- lib/open_project/text_formatting/render_mode.rb | 9 ++++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/app/helpers/mail_formatting_helper.rb b/app/helpers/mail_formatting_helper.rb index abaff32f244..545da16ca2c 100644 --- a/app/helpers/mail_formatting_helper.rb +++ b/app/helpers/mail_formatting_helper.rb @@ -28,11 +28,10 @@ # See COPYRIGHT and LICENSE files for more details. #++ -# Mailer-view wrappers around {OpenProject::TextFormatting#format_text}. They -# 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` template extension to the helper name -# keeps caller intent visible at the call site. +# 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, **) diff --git a/lib/open_project/text_formatting/render_mode.rb b/lib/open_project/text_formatting/render_mode.rb index 3305c629b73..fd78397b739 100644 --- a/lib/open_project/text_formatting/render_mode.rb +++ b/lib/open_project/text_formatting/render_mode.rb @@ -34,11 +34,10 @@ module OpenProject # `:external_text`) onto the primitive `only_path` / `static_html` / # `plain_text` context flags that the filter pipeline reads. # - # The collapse exists because external surfaces (mailers today; RSS, PDF, - # webhooks tomorrow) always need absolute URLs *and* static rendering for - # JS-dependent components — the two flags never travel apart in practice. - # A single mode value is the canonical API; the primitives stay available - # as per-flag escape hatches for callers that need an asymmetric mix. + # 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,