From 520738ddc4e3d4277d45a678a1d9e5190e513ced Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 15 May 2026 14:23:43 +0300 Subject: [PATCH] Accept semantic work-package identifiers in text macros Broaden the bare `#` branch of the text-macro regex to accept both the numeric (`\d+`) and semantic (`[A-Z][A-Z0-9_]*-\d+`) identifier shapes, reusing `WorkPackage::SemanticIdentifier::ID_ROUTE_CONSTRAINT` as the single source of truth. The link handler interpolates the typed identifier verbatim into the href, label, and `data-id`; the route layer resolves either shape transparently via `find_by_display_id`, so no DB lookup is needed at render time and the behaviour is mode-agnostic. `#PROJ-1` renders as `#PROJ-1` and `##PROJ-1` / `###PROJ-1` render as `` elements with `data-id="PROJ-1"`. Numeric `#1234` rendering is unchanged. The leading-zero canonical-form guard (`#0123` stays literal) is preserved on the numeric branch. Two collateral safety guards: - `HashSeparator#applicable?` rejects semantic-shape inputs digit-by- digit so `version#PROJ-1` (and every other prefixed-resource form) short-circuits before issuing a guaranteed-miss `find_by(id: 0)` against the versions / documents / messages / meetings / projects / users / groups / queries tables. - The PDF export's `WorkPackagesLinkHandler` gates `applicable?` on digit-only identifiers. PDF rendering walks a separate Markly pipeline that does not yet resolve semantic ids; without this guard, `#PROJ-1` in an exported body would collapse to ``. Semantic-id PDF support is a follow-up (community WP #74366 / #74766). Refs https://community.openproject.org/wp/74948 --- .../work_package/exports/macros/links.rb | 9 +- .../matchers/link_handlers/hash_separator.rb | 5 +- .../matchers/link_handlers/work_packages.rb | 31 ++--- .../matchers/resource_links_matcher.rb | 81 ++++++------ .../link_handlers/work_packages_spec.rb | 122 ++++++++++++++++++ spec/models/exports/pdf/common/macro_spec.rb | 24 ++++ 6 files changed, 215 insertions(+), 57 deletions(-) create mode 100644 spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb diff --git a/app/models/work_package/exports/macros/links.rb b/app/models/work_package/exports/macros/links.rb index d0022fe3bf97..35033e1c5c5d 100644 --- a/app/models/work_package/exports/macros/links.rb +++ b/app/models/work_package/exports/macros/links.rb @@ -31,8 +31,15 @@ module WorkPackage::Exports module Macros class WorkPackagesLinkHandler < OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages + # PDF export walks a separate Markly pipeline that does not yet resolve + # semantic identifiers. Rejecting the semantic shape here keeps + # `#PROJ-1` as literal text in PDFs rather than emitting a broken + # ``. See community WP #74366 / #74766 for the + # follow-up that adds semantic-id support to the PDF side. def applicable? - %w(# ## ###).include?(matcher.sep) && matcher.prefix.blank? + %w(# ## ###).include?(matcher.sep) && + matcher.prefix.blank? && + matcher.identifier&.match?(/\A\d+\z/) end def render_link(wp_id, matcher) diff --git a/lib/open_project/text_formatting/matchers/link_handlers/hash_separator.rb b/lib/open_project/text_formatting/matchers/link_handlers/hash_separator.rb index e683d9fc1e9f..444d9e87430e 100644 --- a/lib/open_project/text_formatting/matchers/link_handlers/hash_separator.rb +++ b/lib/open_project/text_formatting/matchers/link_handlers/hash_separator.rb @@ -39,8 +39,11 @@ def self.allowed_prefixes # Hash-separated object links # Condition: Separator is '#' # Condition: Prefix is present, checked to be one of the allowed values + # Condition: Identifier is digit-only — semantic-shape inputs like + # `version#PROJ-1` short-circuit here so they don't issue a guaranteed- + # miss `find_by(id: 0)` against every prefixed resource table. def applicable? - matcher.sep == "#" && valid_prefix? && oid.present? + matcher.sep == "#" && valid_prefix? && matcher.identifier&.match?(/\A\d+\z/) end # Examples: 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 a55e78144c38..73b79eb7db1f 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 @@ -42,40 +42,41 @@ def applicable? # # Examples: # - # #1234, ##1234, ###1234 + # #1234, ##1234, ###1234, #PROJ-1, ##PROJ-1, ###PROJ-1 def call - wp_id = matcher.identifier.to_i + identifier = matcher.identifier - # Ensure that the element was entered numeric, - # prohibits links to things like #0123 - return if wp_id.to_s != matcher.identifier + # Reject canonical-shape violations on the numeric branch so `#0123` + # stays literal instead of resolving to WP 123. The semantic branch is + # already shape-validated by the matcher regex. + return if identifier.match?(/\A\d+\z/) && identifier.to_i.to_s != identifier - render_link(wp_id, matcher) + render_link(identifier, matcher) end - def render_link(wp_id, matcher) + def render_link(identifier, matcher) if ["##", "###"].include?(matcher.sep) - render_work_package_macro(wp_id, detailed: (matcher.sep === "###")) + render_work_package_macro(identifier, detailed: (matcher.sep === "###")) else - render_work_package_link(wp_id) + render_work_package_link(identifier) end end private - def render_work_package_macro(wp_id, detailed: false) + def render_work_package_macro(identifier, detailed: false) ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", - data: { id: wp_id, detailed: } + data: { id: identifier, detailed: } end - def render_work_package_link(wp_id) - link_to("##{wp_id}", - work_package_path_or_url(id: wp_id, only_path: context[:only_path]), + def render_work_package_link(identifier) + link_to("##{identifier}", + work_package_path_or_url(id: identifier, only_path: context[:only_path]), class: "issue work_package", data: { hover_card_trigger_target: "trigger", - hover_card_url: hover_card_work_package_path(wp_id) + hover_card_url: hover_card_work_package_path(identifier) }) 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 3ec9352628ac..402259378cff 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -33,8 +33,14 @@ module Matchers # OpenProject links matching # # Examples: - # Issues: - # #52 -> Link to issue #52 + # Work packages: + # #52 -> Plain link to work package 52 + # ##52 -> Inline quickinfo card for work package 52 + # ###52 -> Detailed quickinfo card for work package 52 + # Work packages (semantic identifiers): + # #PROJ-7 -> Plain link to the work package whose display id is PROJ-7 + # ##PROJ-7 -> Inline quickinfo card + # ###PROJ-7 -> Detailed quickinfo card # Changesets: # r52 -> Link to revision 52 # commit:a85130f -> Link to scmid starting with a85130f @@ -76,35 +82,28 @@ class ResourceLinksMatcher < RegexMatcher include ActionView::Helpers::TextHelper include ActionView::Helpers::UrlHelper + # Hash and revision separators sit on independent alternation branches + # so semantic ids apply only to `#` references — `r` revisions stay + # numeric-only. def self.regexp + semantic_id = WorkPackage::SemanticIdentifier::ID_ROUTE_CONSTRAINT.source + prefixes = allowed_prefixes.join("|") %r{ - ([[[:space:]](,~\-\[>]|^) # Leading string - (!)? # Escaped marker - (([a-z0-9\-_]+):)? # Project identifier - (#{allowed_prefixes.join('|')})? # prefix - ( - (\#+|r)(\d+) # separator and its identifier + (?[[[:space:]](,~\-\[>]|^) + (?!)? + (?(?[a-z0-9\-_]+):)? + (?#{prefixes})? + (?: + (?\#+)(?#{semantic_id}) | - (:) # or colon separator - ( - [^"\s<>][^\s<>]*? # And a non-quoted value [10] - | - "([^"]+)" # Or a quoted value [11] - ) + (?r)(?\d+) + | + (?:)(?[^"\s<>][^\s<>]*?|"(?[^"]+)") ) (?= - (?= - [[:punct:]]\W # Includes matches of, e.g., source:foo.ext - ) - |\.\z # Allow matching when string ends with . - |, # or with , - |~ # or with ~ - |\) # or with ) - |[[:space:]] - |\] - |< - |$ - ) + (?=[[:punct:]]\W) # Includes matches of, e.g., source:foo.ext + |\.\z|,|~|\)|[[:space:]]|\]|<|$ + ) }x end @@ -128,21 +127,23 @@ def self.link_handlers ] end - def self.process_match(m, matched_string, context) - # Leading string before match - instance = new( - matched_string:, - leading: m[1], - escaped: m[2], - project_prefix: m[3], - project_identifier: m[4], - prefix: m[5], - sep: m[7] || m[9], - raw_identifier: m[8] || m[10], - identifier: m[8] || m[11] || m[10], - context: - ) + # Flattens the three alternation branches into a single `:sep` / + # `:identifier` shape so callers don't branch on which one matched. + def self.parse_match(match) + { + leading: match[:leading], + escaped: match[:escaped], + project_prefix: match[:project_prefix], + project_identifier: match[:project_identifier], + prefix: match[:prefix], + sep: match[:hash_sep] || match[:rev_sep] || match[:colon_sep], + raw_identifier: match[:hash_id] || match[:rev_id] || match[:colon_value], + identifier: match[:hash_id] || match[:rev_id] || match[:quoted] || match[:colon_value] + } + end + def self.process_match(matchdata, matched_string, context) + instance = new(matched_string:, context:, **parse_match(matchdata)) instance.process 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 new file mode 100644 index 000000000000..d6992dd56aec --- /dev/null +++ b/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb @@ -0,0 +1,122 @@ +# 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" +require_relative "../../markdown/expected_markdown" + +RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages do + include_context "expected markdown modules" + + shared_let(:user) { create(:admin) } + + before { allow(User).to receive(:current).and_return(user) } + + # WP-table SELECTs from any helper invocation other than `format_text` + # itself — model setup, current-user fetches, project preloads — must not + # leak into the regression guard. Wrap only the rendering call. + def work_package_selects_during(&) + recorder = ActiveRecord::QueryRecorder.new(&) + recorder.log.grep(/FROM "work_packages"/i) + end + + describe "the `#N` numeric reference" do + let(:rendered) { format_text("#1234") } + + it "renders an anchor with the typed identifier in both the label and href" do + expect(rendered).to include(">#1234<") + expect(rendered).to include(%(href="/work_packages/1234")) + end + + it "does not issue any work_packages SELECTs" do + selects = work_package_selects_during { format_text("#1234") } + expect(selects).to be_empty + end + + context "with a leading-zero numeric form" do + it "stays as literal text (e.g. `#0123` does not resolve to WP 123)" do + result = format_text("#0123") + expect(result).to include("#0123") + expect(result).not_to include(%(href="/work_packages/)) + end + end + end + + describe "the `#PROJ-N` semantic reference" do + let(:rendered) { format_text("#PROJ-1") } + + it "renders an anchor with the typed identifier in both the label and href" do + expect(rendered).to include(">#PROJ-1<") + expect(rendered).to include(%(href="/work_packages/PROJ-1")) + end + + it "does not issue any work_packages SELECTs (route resolves both shapes)" do + selects = work_package_selects_during { format_text("#PROJ-1") } + expect(selects).to be_empty + end + + it "renders identically in classic and semantic mode (routing is mode-agnostic)" do + Setting.work_packages_identifier = "classic" + classic = format_text("#PROJ-1") + + Setting.work_packages_identifier = "semantic" + semantic = format_text("#PROJ-1") + + expect(classic).to eq(semantic) + end + end + + describe "the `##PROJ-N` quickinfo reference" do + it "emits an inline quickinfo macro element with the typed identifier" do + # Prepend "see " so Markly doesn't parse `##…` as an H2 ATX heading. + rendered = format_text("see ##PROJ-1 here") + + expect(rendered).to include(%()) + end + + it "emits a detailed quickinfo macro element for `###PROJ-N`" do + rendered = format_text("see ###PROJ-1 here") + + expect(rendered).to include(%()) + end + + it "issues no work_packages SELECTs" do + selects = work_package_selects_during { format_text("see ##PROJ-1 and ###PROJ-2 here") } + expect(selects).to be_empty + end + end + + describe "prefixed `version#…` references" do + it "does not query the versions table when the identifier is semantic-shaped" do + recorder = ActiveRecord::QueryRecorder.new { format_text("see version#PROJ-1 here") } + version_selects = recorder.log.grep(/FROM "versions"/i) + expect(version_selects).to be_empty + end + end +end diff --git a/spec/models/exports/pdf/common/macro_spec.rb b/spec/models/exports/pdf/common/macro_spec.rb index 00fc85c4f466..61db45df32a3 100644 --- a/spec/models/exports/pdf/common/macro_spec.rb +++ b/spec/models/exports/pdf/common/macro_spec.rb @@ -200,6 +200,30 @@ expect(formatted).to eq("

#{expected_tag}

") end end + + describe "with a semantic-shape reference" do + # PDF export walks a separate Markly pipeline that does not yet resolve + # semantic identifiers. Without the numeric-only guard, `#PROJ-1` would + # collapse to `` (since "PROJ-1".to_i == 0). + # Falling through to literal text is the correct interim behaviour + # pending PDF support for semantic ids. + let(:markdown) { "#PROJ-1" } + + it "falls through to literal text and never emits a `data-id=\"0\"` mention" do + expect(formatted).not_to include('data-id="0"') + expect(formatted).to include("PROJ-1") + end + end + + describe "with mixed numeric and semantic references" do + let(:markdown) { "##{work_package.id} and #PROJ-1" } + + it "resolves the numeric reference and leaves the semantic one literal" do + expect(formatted).to include(expected_tag) + expect(formatted).to include("#PROJ-1") + expect(formatted).not_to include('data-id="0"') + end + end end describe "workPackageValue macro" do