diff --git a/app/models/work_package/exports/macros/links.rb b/app/models/work_package/exports/macros/links.rb index d0022fe3bf97..f11fde27667a 100644 --- a/app/models/work_package/exports/macros/links.rb +++ b/app/models/work_package/exports/macros/links.rb @@ -31,8 +31,21 @@ module WorkPackage::Exports module Macros class WorkPackagesLinkHandler < OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages + # PDF export only handles canonical numeric `#N` references. Semantic and + # leading-zero shapes fall through to literal text to avoid emitting a + # `` (since `"PROJ-1".to_i == 0`). Semantic-id + # support in PDF export is tracked in https://community.openproject.org/wp/74766. def applicable? - %w(# ## ###).include?(matcher.sep) && matcher.prefix.blank? + hash_trigger? && + matcher.prefix.blank? && + WorkPackage::SemanticIdentifier.numeric_id?(matcher.identifier) + end + + # PDF rendering walks Markly nodes directly and bypasses the + # `PatternMatcherFilter` preload pipeline, so the parent's cache-driven + # `call` would miss every reference. + def call + render_link(matcher.identifier.to_i, matcher) 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..1d3555bc005f 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 @@ -35,12 +35,11 @@ def self.allowed_prefixes %w(version message project user group document meeting view) end - ## - # Hash-separated object links - # Condition: Separator is '#' - # Condition: Prefix is present, checked to be one of the allowed values + # Digit-only ids parse via `to_i` into primary keys. Semantic-shaped + # inputs (`version#PROJ-1`) short-circuit here so they don't issue + # `find_by(id: 0)`. 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..f975af718a79 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 @@ -31,51 +31,94 @@ module OpenProject::TextFormatting::Matchers module LinkHandlers class WorkPackages < Base - ## - # Match work package links. - # Condition: Separator is #|##|### - # Condition: Prefix is nil + # CKEditor `#`-based mention triggers for WP references: `#N` plain link, + # `##N` compact quickinfo, `###N` detailed quickinfo. Distinct from the + # matcher's generic `sep` vocabulary (where `#` *separates* prefix from + # id in `version#3`); here it's a sigil that triggers mention recognition. + # Shared with the PDF-export subclass in `app/models/work_package/exports/macros/links.rb`. + HASH_TRIGGERS = %w[# ## ###].freeze + def applicable? - %w(# ## ###).include?(matcher.sep) && matcher.prefix.nil? + hash_trigger? && matcher.prefix.nil? end - # - # Examples: - # - # #1234, ##1234, ###1234 + # Examples: #1234, ##1234, ###1234, #PROJ-7, ##PROJ-7, ###PROJ-7 def call - wp_id = matcher.identifier.to_i + identifier = matcher.identifier + + if WorkPackage::SemanticIdentifier.semantic_id?(identifier) + # Semantic shapes are only meaningful in semantic mode; classic + # instances render the literal text fallback. + return nil unless Setting::WorkPackageIdentifier.semantic_mode_active? + + render_for_semantic(identifier) + else + # Reject leading-zero shapes like `#0123` that aren't canonical PK strings. + return nil unless WorkPackage::SemanticIdentifier.numeric_id?(identifier) + + render_for_numeric(identifier.to_i) + end + end - # Ensure that the element was entered numeric, - # prohibits links to things like #0123 - return if wp_id.to_s != matcher.identifier + private + + def hash_trigger? + HASH_TRIGGERS.include?(matcher.sep) + end - render_link(wp_id, matcher) + def quickinfo? + matcher.sep.length > 1 end - def render_link(wp_id, matcher) - if ["##", "###"].include?(matcher.sep) - render_work_package_macro(wp_id, detailed: (matcher.sep === "###")) + def detailed? + matcher.sep == "###" + end + + def render_for_semantic(display_id) + if quickinfo? + render_work_package_macro(display_id, detailed: detailed?) else - render_work_package_link(wp_id) + # Plain link needs the WP record for the `formatted_id` label and + # hover-card URL. Unresolved → literal text rather than a broken URL. + wp = WorkPackage.find_by_display_id(display_id) + return nil unless wp + + render_work_package_link(wp, fallback_id: display_id) end end - private + def render_for_numeric(wp_id) + wp = WorkPackage.find_by_display_id(wp_id) - def render_work_package_macro(wp_id, detailed: false) + if quickinfo? + # Prefer the resolved WP's display_id so `##1234` typed in semantic + # mode renders the user-facing identifier in the editor model. + data_id = wp&.display_id || wp_id + render_work_package_macro(data_id, detailed: detailed?) + else + render_work_package_link(wp, fallback_id: wp_id) + end + end + + def render_work_package_macro(data_id, detailed: false) ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", - data: { id: wp_id, detailed: } + data: { id: data_id, 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(work_package, fallback_id:) + # Fall back to the bare `#N` shape when no WP is provided (classic mode, + # render path bypassing `PatternMatcherFilter`) rather than running a + # per-link query inside the renderer. + label = work_package&.formatted_id || "##{fallback_id}" + href_id = work_package&.display_id || fallback_id + + link_to(label, + work_package_path_or_url(id: href_id, 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(href_id) }) 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..1e3b02a49a29 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, when the instance is in semantic mode): + # #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/markdown/in_tool_links_spec.rb b/spec/lib/open_project/text_formatting/markdown/in_tool_links_spec.rb index cbbfc0cd541d..3d0e2774c8df 100644 --- a/spec/lib/open_project/text_formatting/markdown/in_tool_links_spec.rb +++ b/spec/lib/open_project/text_formatting/markdown/in_tool_links_spec.rb @@ -147,6 +147,12 @@ it { is_expected.to be_html_eql("

#{version_link}

") } end + context "with a leading-zero version id" do + subject { format_text("version#0#{version.id}") } + + it { is_expected.to be_html_eql("

#{version_link}

") } + end + context "Link with version" do subject { format_text("version:1.0") } 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..0ecf3dc94768 --- /dev/null +++ b/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb @@ -0,0 +1,237 @@ +# frozen_string_literal: true + +require "spec_helper" +require_relative "../../markdown/expected_markdown" + +RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages do + include_context "expected markdown modules" + + # Boilerplate for the typical "current user can view a WP in this project" + # setup. Consumers must define `:project`; `author` is realised eagerly via + # the stubbed `User.current` so role/membership creation runs once per + # example. + shared_context "with author signed in" do + let(:role) { create(:project_role, permissions: %i[view_work_packages]) } + let(:author) { create(:user, member_with_roles: { project => role }) } + + before { allow(User).to receive(:current).and_return(author) } + end + + describe "the `#N` plain reference" do + include_context "with author signed in" + let(:work_package) { create(:work_package, project:, author:) } + + context "in classic mode", + with_flag: { semantic_work_package_ids: false }, + with_settings: { work_packages_identifier: "classic" } do + let(:project) { create(:project, identifier: "macroproj") } + + it "renders the numeric id with a `#` prefix and a numeric href" do + rendered = format_text("##{work_package.id}") + + expect(rendered).to include(">##{work_package.id}<") + expect(rendered).to include(%(href="/work_packages/#{work_package.id}")) + end + end + + context "in semantic mode", + with_flag: { semantic_work_package_ids: true }, + with_settings: { work_packages_identifier: "semantic" } do + let(:project) { create(:project, identifier: "MACROPROJ") } + + before { work_package.allocate_and_register_semantic_id } + + it "renders the formatted_id (PROJ-N) and the displayId in the href" do + wp = work_package.reload + rendered = format_text("##{wp.id}") + + expect(wp.formatted_id).to start_with("MACROPROJ-") + expect(rendered).to include(">#{wp.formatted_id}<") + expect(rendered).to include(%(href="/work_packages/#{wp.display_id}")) + expect(rendered).not_to include(">##{wp.id}<") + end + end + + context "when the referenced work package does not exist", + with_flag: { semantic_work_package_ids: true }, + with_settings: { work_packages_identifier: "semantic" } do + let(:project) { create(:project, identifier: "MACROPROJ") } + + it "falls back to the numeric label and href (no DB error)" do + # Realise project + author so format_text has a current user, but + # do not realise work_package — render a `#N` reference whose id + # has no matching record. + project + author + + rendered = format_text("#999999") + + expect(rendered).to include(">#999999<") + expect(rendered).to include(%(href="/work_packages/999999")) + end + end + end + + describe "per-reference query cost", + with_flag: { semantic_work_package_ids: true }, + with_settings: { work_packages_identifier: "semantic" } do + include_context "with author signed in" + let(:project) { create(:project, identifier: "NPLUSONE") } + + it "issues one work_packages SELECT per `#N` reference" 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(5), + "expected one SELECT per reference, got #{wp_selects.size}:\n#{wp_selects.join("\n")}" + end + end + + describe "the `#PROJ-N` semantic reference", + with_flag: { semantic_work_package_ids: true }, + with_settings: { work_packages_identifier: "semantic" } do + include_context "with author signed in" + 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 label and display_id href for `#PROJ-N`" do + wp = work_package.reload + rendered = format_text("##{wp.display_id}") + + expect(wp.display_id).to start_with("MACROPROJ-") + expect(rendered).to include(">#{wp.formatted_id}<") + expect(rendered).to include(%(href="/work_packages/#{wp.display_id}")) + # The hover-card route accepts both numeric and semantic ids; pass + # display_id so the URL matches the user-facing identifier. + expect(rendered).to include(%(data-hover-card-url="/work_packages/#{wp.display_id}/hover_card")) + end + + it "renders `##PROJ-N` as a quickinfo macro element with display_id in data-id" do + wp = work_package.reload + # Prepend "see " so Markly doesn't parse `##...` as an H2 ATX heading. + rendered = format_text("see ###{wp.display_id} here") + + expect(rendered).to include(%()) + end + + it "renders `###PROJ-N` as a detailed quickinfo macro element" do + wp = work_package.reload + rendered = format_text("see ####{wp.display_id} here") + + expect(rendered).to include(%()) + end + + context "when the referenced work package does not exist" do + it "falls back to literal text (no DB error, no broken link)" do + rendered = format_text("see #GHOST-99 here") + + # The matcher leaves the literal text alone when a semantic-shaped + # reference can't be resolved — better than emitting a link to + # `/work_packages/GHOST-99` that 404s. + expect(rendered).to include("#GHOST-99") + expect(rendered).not_to include('href="/work_packages/GHOST-99"') + expect(rendered).not_to include("opce-macro-wp-quickinfo") + end + end + + context "with mixed numeric and semantic references in one render" do + it "resolves both with one work_packages SELECT per reference" do + wps = create_list(:work_package, 2, project:, author:) + wps.each(&:allocate_and_register_semantic_id) + loaded = wps.map(&:reload) + text = "see ##{loaded[0].id} and ##{loaded[1].display_id}" + + rendered = nil + recorder = ActiveRecord::QueryRecorder.new { rendered = format_text(text) } + wp_selects = recorder.log.grep(/FROM "work_packages"/i) + + expect(wp_selects.size).to eq(2), + "expected one SELECT per reference, got #{wp_selects.size}:\n#{wp_selects.join("\n")}" + + # Both render with the user-facing display_id, regardless of which + # form the user typed. + expect(rendered).to include(%(href="/work_packages/#{loaded[0].display_id}")) + expect(rendered).to include(%(href="/work_packages/#{loaded[1].display_id}")) + end + end + + context "with a historical alias reference" do + it "resolves via the alias table without a separate alias round-trip" 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 + # points at the same WP. Authors writing pre-rename content + # shouldn't see broken refs. + WorkPackageSemanticAlias.create!(work_package_id: wp.id, identifier: "OLDPROJ-1") + + rendered = nil + recorder = ActiveRecord::QueryRecorder.new { rendered = format_text("see #OLDPROJ-1") } + + # `find_by_display_id` runs one WP SELECT whose WHERE clause is + # `identifier = ? OR EXISTS (SELECT 1 FROM aliases …)`, so the + # alias table is consulted in-line — no separate alias-only SELECT. + wp_selects = recorder.log.grep(/FROM "work_packages"/i) + alias_only_selects = recorder.log.grep(/FROM "work_package_semantic_aliases"/i) + .grep_v(/FROM "work_packages"/i) + expect(wp_selects.size).to eq(1) + expect(alias_only_selects).to be_empty + + # Renders against the WP's CURRENT display_id, not the historical + # alias the user typed — old content stays alive but points at the + # current identifier. + expect(rendered).to include(%(href="/work_packages/#{wp.display_id}")) + expect(rendered).to include(">#{wp.formatted_id}<") + end + end + end + + describe "the `#PROJ-N` semantic reference in classic mode", + with_flag: { semantic_work_package_ids: false }, + with_settings: { work_packages_identifier: "classic" } do + include_context "with author signed in" + let(:project) { create(:project, identifier: "macroproj") } + + it "leaves `#PROJ-1` as literal text and issues no work_packages SELECTs" do + rendered = nil + recorder = ActiveRecord::QueryRecorder.new { rendered = format_text("see #PROJ-1 here") } + wp_selects = recorder.log.grep(/FROM "work_packages"/i) + + expect(rendered).to include("#PROJ-1") + expect(rendered).not_to include('href="/work_packages/PROJ-1"') + expect(wp_selects).to be_empty, + "classic mode added unexpected WP SELECTs for semantic input:\n#{wp_selects.join("\n")}" + end + end + + describe "prefixed resource refs with semantic-shaped identifiers", + with_flag: { semantic_work_package_ids: true }, + with_settings: { work_packages_identifier: "semantic" } do + # `version#PROJ-1`, `message#PROJ-1`, etc. share the regex with the WP + # macro because `\d+|PROJ-\d+` is a single alternation. The prefixed + # branches address tables keyed by numeric primary key, so a semantic + # identifier paired with a prefix must short-circuit before the handler + # would otherwise issue `find_by(id: 0)` (the round-trip of any + # non-numeric string through `to_i`). + include_context "with author signed in" + let(:project) { create(:project, identifier: "MACROPROJ") } + + it "does not query the prefixed resource table for `version#PROJ-1`" do + project + author + + rendered = nil + recorder = ActiveRecord::QueryRecorder.new { rendered = format_text("see version#PROJ-1 here") } + version_selects = recorder.log.grep(/FROM "versions"/i) + + expect(version_selects).to be_empty, + "expected zero versions SELECTs for semantic-shaped input, got:\n" \ + "#{version_selects.join("\n")}" + expect(rendered).to include("version#PROJ-1") + 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..4cacb4a530f5 100644 --- a/spec/models/exports/pdf/common/macro_spec.rb +++ b/spec/models/exports/pdf/common/macro_spec.rb @@ -200,6 +200,33 @@ expect(formatted).to eq("

#{expected_tag}

") end end + + # Semantic-id support in PDF export is tracked separately in + # https://community.openproject.org/wp/74766. Until that ships, any + # `#PROJ-1`-shaped reference must fall through to literal text in PDF + # output rather than emitting `` (since + # `"PROJ-1".to_i == 0`). + describe "with semantic identifier (out of scope per WP #74766)" do + describe "alone" do + let(:markdown) { "see #PROJ-1 here" } + + it "falls through to literal text without emitting a mention" do + expect(formatted).to include("#PROJ-1") + expect(formatted).not_to include('data-id="0"') + expect(formatted).not_to include("