Use formatted_id and displayId in WP MentionFilter rendering

frontend/src/app/shared/components/fields/macros/work-package-quickinfo-macro.component.ts

@@ -91,7 +91,9 @@ export class WorkPackageQuickinfoMacroComponent implements OnInit {
 
   ngOnInit() {
     const element = this.elementRef.nativeElement as HTMLElement;
-    const id:string = element.dataset.id!;
+    // Prefer `data-display-id`; fall back to `data-id` for legacy
+    // stored markdown emitted before the attribute split.
+    const id:string = element.dataset.displayId ?? element.dataset.id!;
     this.detailed = element.dataset.detailed === 'true';
     this.workPackageHoverCardUrl = this.pathHelper.workPackageHoverCardPath(id);
 

lib/open_project/text_formatting/filters/mention_filter.rb

@@ -75,22 +75,26 @@ def group_mention(group)
       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.
+        href_id = work_package.display_id
+
         case mention.text.count("#")
         when 3
           ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo",
                                                     "",
-                                                    data: { id: work_package.id, detailed: true }
+                                                    data: { id: work_package.id, display_id: href_id, detailed: true }
         when 2
           ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo",
                                                     "",
-                                                    data: { id: work_package.id, detailed: false }
+                                                    data: { id: work_package.id, display_id: href_id, detailed: false }
         else
-          link_to("##{work_package.id}",
-                  work_package_path_or_url(id: work_package.id, only_path: context[:only_path]),
+          link_to(work_package.formatted_id,
+                  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(work_package.id)
+                    hover_card_url: hover_card_work_package_path(href_id)
                   })
         end
       end
@@ -123,11 +127,10 @@ def fallback_text(mention)
       def controller; end
 
       def mention_id(mention)
-        attribute_value = mention.attributes["data-id"]&.value
-
-        id_match = attribute_value&.match(/\d+/)
-
-        id_match ? id_match[0] : nil
+        value = mention.attributes["data-id"]&.value
+        # Reject semantic-shaped data-ids: `PROJ-42` must not silently
+        # resolve to WP id 42 via embedded-digit extraction.
+        value if value&.match?(/\A\d+\z/)
       end
     end
   end

lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb

@@ -53,7 +53,7 @@ def call
 
           render_for_semantic(identifier)
         else
-          # Reject leading-zero shapes like `#0123` that aren't canonical PK strings.
+          # Reject leading-zero shapes like `#0123` that aren't canonical id strings.
           return nil unless WorkPackage::SemanticIdentifier.numeric_id?(identifier)
 
           render_for_numeric(identifier.to_i)
@@ -75,14 +75,15 @@ def detailed?
       end
 
       def render_for_semantic(display_id)
+        # 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)
+        return nil unless wp
+
         if quickinfo?
-          render_work_package_macro(display_id, detailed: detailed?)
+          render_work_package_macro(id: wp.id, display_id: wp.display_id, detailed: detailed?)
         else
-          # Plain link needs the WP record for the `formatted_id` label and
-          # hover-card URL. Cache miss → literal text rather than a broken URL.
-          wp = OpenProject::TextFormatting::Matchers::ResourceLinksMatcher.work_package_for(display_id)
-          return nil unless wp
-
           render_work_package_link(wp, fallback_id: display_id)
         end
       end
@@ -91,19 +92,20 @@ def render_for_numeric(wp_id)
         wp = OpenProject::TextFormatting::Matchers::ResourceLinksMatcher.work_package_for(wp_id)
 
         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?)
+          # 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?)
         else
           render_work_package_link(wp, fallback_id: wp_id)
         end
       end
 
-      def render_work_package_macro(data_id, detailed: false)
+      def render_work_package_macro(id:, display_id:, detailed: false)
         ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo",
                                                   "",
-                                                  data: { id: data_id, detailed: }
+                                                  data: { id:, display_id:, detailed: }
       end
 
       def render_work_package_link(work_package, fallback_id:)

spec/features/wysiwyg/mentions_spec.rb

@@ -182,35 +182,41 @@
     expect(page).to have_css("span", text: "👍")
   end
 
-  # Each consuming context defines `wp_id` — the value the user types after
-  # the marker, and the value the rendered widget exposes via `data-id` —
-  # and `wp_label`, the visible link label after the macro pipeline renders
-  # the comment.
+  # Each consuming context defines `wp_display_id` — the value the user
+  # types after the marker, which appears on the rendered widget as
+  # `data-display-id` — and `wp_label`, the visible link label after the
+  # macro pipeline renders the comment.
   shared_examples "work package mention with all triggers" do
     it "can autocomplete work packages with different triggers" do
       # Test # trigger
-      activity_tab.type_comment("##{wp_id}")
+      activity_tab.type_comment("##{wp_display_id}")
       page.find(".mention-list-item", text: mentioned_work_package.subject, wait: 10).click
-      expect(page).to have_css("a.mention", text: "##{wp_id}")
+      expect(page).to have_css("a.mention", text: "##{wp_display_id}")
       activity_tab.submit_comment
       activity_tab.expect_journal_notes text: wp_label
 
       # Test ## trigger
-      activity_tab.type_comment("###{wp_id}")
+      activity_tab.type_comment("###{wp_display_id}")
       page.find(".mention-list-item", text: mentioned_work_package.subject, wait: 10).click
       expect(page).to have_css(".op-macro-wp-quickinfo-widget")
       expect(page).to have_css(
-        "opce-macro-wp-quickinfo[data-id='#{wp_id}'][data-detailed='false']"
+        "opce-macro-wp-quickinfo" \
+        "[data-id='#{mentioned_work_package.id}']" \
+        "[data-display-id='#{wp_display_id}']" \
+        "[data-detailed='false']"
       )
       activity_tab.submit_comment
       activity_tab.expect_journal_notes text: "NONE #{wp_label}: #{mentioned_work_package.subject}"
 
       # Test ### trigger
-      activity_tab.type_comment("####{wp_id}")
+      activity_tab.type_comment("####{wp_display_id}")
       page.find(".mention-list-item", text: mentioned_work_package.subject, wait: 10).click
       expect(page).to have_css(".op-macro-wp-quickinfo-widget")
       expect(page).to have_css(
-        "opce-macro-wp-quickinfo[data-id='#{wp_id}'][data-detailed='true']"
+        "opce-macro-wp-quickinfo" \
+        "[data-id='#{mentioned_work_package.id}']" \
+        "[data-display-id='#{wp_display_id}']" \
+        "[data-detailed='true']"
       )
       activity_tab.submit_comment
 
@@ -221,7 +227,7 @@
   context "in classic mode",
           with_flag: { semantic_work_package_ids: false },
           with_settings: { work_packages_identifier: "classic" } do
-    let(:wp_id) { mentioned_work_package.id }
+    let(:wp_display_id) { mentioned_work_package.id }
     let(:wp_label) { "##{mentioned_work_package.id}" }
 
     include_examples "work package mention with all triggers"
@@ -239,7 +245,7 @@
         wp.reload
       end
     end
-    let(:wp_id) { mentioned_work_package.identifier }
+    let(:wp_display_id) { mentioned_work_package.identifier }
     let(:wp_label) { mentioned_work_package.identifier }
 
     include_examples "work package mention with all triggers"

spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb

@@ -0,0 +1,200 @@
+# 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::Filters::MentionFilter do
+  include_context "expected markdown modules"
+
+  describe "work package mention" 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) }
+
+    # Defaults produce the numeric form (`data-id` = primary key,
+    # `data-text` = `#N`).
+    def mention_tag(work_package, sep: "#", data_id: nil, data_text: nil)
+      did = data_id || work_package.id
+      text = data_text || "#{sep}#{work_package.id}"
+      <<~HTML
+        <mention class="mention"
+                 data-id="#{did}"
+                 data-type="work_package"
+                 data-text="#{text}">#{text}</mention>
+      HTML
+    end
+
+    context "as plain link in classic mode",
+            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 numeric `#N` label and a numeric href" do
+        rendered = format_text(mention_tag(work_package))
+
+        expect(rendered).to include(">##{work_package.id}<")
+        expect(rendered).to include(%(href="/work_packages/#{work_package.id}"))
+        expect(rendered).to include(%(data-hover-card-url="/work_packages/#{work_package.id}/hover_card"))
+      end
+    end
+
+    context "as plain link in semantic mode",
+            with_flag: { semantic_work_package_ids: true },
+            with_settings: { work_packages_identifier: "semantic" } do
+      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 the displayId in the href" do
+        wp = work_package.reload
+        rendered = format_text(mention_tag(wp))
+
+        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).to include(%(data-hover-card-url="/work_packages/#{wp.display_id}/hover_card"))
+        expect(rendered).not_to include(">##{wp.id}<")
+      end
+    end
+
+    context "as compact quickinfo (`##`) in semantic mode",
+            with_flag: { semantic_work_package_ids: true },
+            with_settings: { work_packages_identifier: "semantic" } do
+      let(:project) { create(:project, identifier: "MACROPROJ") }
+      let(:work_package) { create(:work_package, project:, author:) }
+
+      before { work_package.allocate_and_register_semantic_id }
+
+      it "emits the quickinfo macro with displayId in data-id" do
+        wp = work_package.reload
+        rendered = format_text(mention_tag(wp, sep: "##"))
+
+        expect(rendered).to include(%(<opce-macro-wp-quickinfo))
+        expect(rendered).to include(%(data-id="#{wp.id}"))
+        expect(rendered).to include(%(data-display-id="#{wp.display_id}"))
+        expect(rendered).to include(%(data-detailed="false"))
+      end
+    end
+
+    context "as detailed quickinfo (`###`) in semantic mode",
+            with_flag: { semantic_work_package_ids: true },
+            with_settings: { work_packages_identifier: "semantic" } do
+      let(:project) { create(:project, identifier: "MACROPROJ") }
+      let(:work_package) { create(:work_package, project:, author:) }
+
+      before { work_package.allocate_and_register_semantic_id }
+
+      it "emits the detailed quickinfo macro with displayId in data-id" do
+        wp = work_package.reload
+        rendered = format_text(mention_tag(wp, sep: "###"))
+
+        expect(rendered).to include(%(<opce-macro-wp-quickinfo))
+        expect(rendered).to include(%(data-id="#{wp.id}"))
+        expect(rendered).to include(%(data-display-id="#{wp.display_id}"))
+        expect(rendered).to include(%(data-detailed="true"))
+      end
+    end
+
+    context "as compact quickinfo (`##`) in classic mode",
+            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 "emits the quickinfo macro with the numeric id (no regression)" do
+        rendered = format_text(mention_tag(work_package, sep: "##"))
+
+        expect(rendered).to include(%(<opce-macro-wp-quickinfo))
+        expect(rendered).to include(%(data-id="#{work_package.id}"))
+        expect(rendered).to include(%(data-display-id="#{work_package.id}"))
+        expect(rendered).to include(%(data-detailed="false"))
+      end
+    end
+
+    # Classic-mode rendering stays numeric even when the WP itself carries
+    # a semantic identifier. Labels and URLs key off the mode, not the
+    # record state.
+    context "in classic mode when the WP carries a semantic identifier",
+            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:) }
+      let(:wp) { work_package.reload }
+
+      before { work_package.allocate_and_register_semantic_id }
+
+      it "renders the numeric `#N` label even when the WP has a semantic identifier" do
+        rendered = format_text(mention_tag(wp))
+
+        expect(wp.identifier).to be_present
+        expect(rendered).to include(">##{wp.id}<")
+        expect(rendered).to include(%(href="/work_packages/#{wp.id}"))
+        expect(rendered).not_to include(wp.identifier)
+      end
+    end
+
+    context "with an unresolvable data-id",
+            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 "falls back to the literal mention text without crashing" do
+        tag = mention_tag(work_package, data_id: "999999999", data_text: "#999999999")
+
+        expect { format_text(tag) }.not_to raise_error
+        expect(format_text(tag)).to include("#999999999")
+      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",
+            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 "falls back to the literal mention text without resolving the wrong record" do
+        tag = mention_tag(work_package,
+                          data_id: "PROJ-#{work_package.id}",
+                          data_text: "#PROJ-#{work_package.id}")
+
+        rendered = format_text(tag)
+        expect(rendered).to include("#PROJ-#{work_package.id}")
+        expect(rendered).not_to include(%(/work_packages/#{work_package.id}))
+      end
+    end
+  end
+end