Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,31 @@ module Wikis::Concerns
module UpdateReverseInlineWikiPageLinks
extend ActiveSupport::Concern

# Mirrors the prefix character class of the inline-text macro matcher.
# The trailing `(?!\w)` on the semantic branch keeps `#PROJ-1abc` from
# matching `#PROJ-1`; the numeric branch deliberately has no trailing
# boundary to preserve historic behaviour for inputs like `#13-blubb`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lack of boundary for numeric IDs is interesting and worth a separate non-blocking discussion. Do we actually want to retain that behaviour?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we should retain it, but I'm hesitant to disrupt it in this swing. This test shows how it can be used in classic mode i.e. "Trailing: #1234abc"

# rubocop:disable Style/RedundantRegexpEscape
WP_REF_RE = /
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

(?:[[:space:],~>\#\(\[\-]|^)\#
(?:
(\d+)
|
(#{WorkPackage::SemanticIdentifier::SEMANTIC_ID_PATTERN.source})(?!\w)
)
/x
# rubocop:enable Style/RedundantRegexpEscape

def update_reverse_inline_wiki_page_links(wiki_page)
provider = Wikis::InternalProvider.enabled.first
return if provider.nil?

Wikis::ReverseInlinePageLink.where(provider:, identifier: wiki_page.id).delete_all

find_wp_links(wiki_page.text).uniq.each do |wp_id|
wp = WorkPackage.find_by(id: wp_id)
next if wp.nil?
identifiers = find_wp_links(wiki_page.text).uniq
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should we also somehow uniq distinct references pointing to the same WP? e.g. scenarios where 123, OLDPROJ-10 and NEWPROJ-5 all resolve to the same record.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, I suppose we can take care of that at the where_display_id_in scope- as 123, OLDPROJ-10 and NEWPROJ-5 should resolve to the same work package record

Copy link
Copy Markdown
Member Author

@akabiru akabiru May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 TDD'd this — the failing test went green on first run, so no production change.

Added 22c70f9 as regression coverage: a body with #<id>, #<current-identifier>, and #OLD-<seq> (alias) all pointing at the same WP produces exactly one reverse-link row.

The reason it already works: WorkPackage.where_display_id_in composes the three lookups as WHERE id IN (…) OR identifier IN (…) OR EXISTS(alias …). Single SELECT over work_packages, set semantics, so find_each yields the WP once and create! fires once. Adding .distinct on the scope would force a redundant SELECT DISTINCT pass on every call without changing the result.

return if identifiers.empty?

WorkPackage.where_display_id_in(identifiers).find_each do |wp|
Wikis::ReverseInlinePageLink.create!(linkable: wp, provider:, identifier: wiki_page.id)
end
end
Expand All @@ -51,9 +66,7 @@ def update_reverse_inline_wiki_page_links(wiki_page)
def find_wp_links(text)
return [] if text.blank?

# extracted prefix from lib/open_project/text_formatting/matchers/resource_links_matcher.rb
# adding # as additional prefix
text.scan(/(?:[[:space:],~>#\(\[\-]|^)#([0-9]+)/) # rubocop:disable Style/RedundantRegexpEscape
text.scan(WP_REF_RE).map { |numeric, semantic| numeric || semantic }
end
end
end
102 changes: 102 additions & 0 deletions modules/wikis/spec/services/wiki_pages/create_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,20 @@
end
end

context "when a numeric reference is immediately followed by alphanumeric text" do
# The numeric branch of WP_REF_RE has no trailing `(?!\w)` boundary —
# historic behaviour matches `#13` inside `#13-blubb` and similar
# shapes. Locked here so a future tightening of the boundary can't
# silently strip reverse-links from existing wiki content.
let(:text) { "Trailing: ##{work_package.id}abc" }

it "still creates a reverse page link from the numeric prefix" do
subject

expect(reverse_link_finder.count).to eq(1)
end
end

context "when the internal provider is disabled" do
let(:internal_provider) { create(:internal_wiki_provider, enabled: false) }

Expand All @@ -202,4 +216,92 @@
expect(Wikis::ReverseInlinePageLink.count).to eq(0)
end
end

context "with a semantic-identifier reference",
Comment thread
akabiru marked this conversation as resolved.
with_flag: { semantic_work_package_ids: true },
with_settings: { work_packages_identifier: "semantic" } do
let(:project) { create(:project, :semantic) }
let(:work_package) do
create(:work_package, project:).tap do |wp|
wp.allocate_and_register_semantic_id
wp.reload
end
end

context "when the reference uses the semantic identifier" do
let(:text) { "See ##{work_package.identifier} for context." }

it "creates a reverse page link" do
subject

expect(reverse_link_finder.count).to eq(1)
end
end

context "when the semantic reference uses the ## widget syntax" do
let(:text) { "Block: ###{work_package.identifier}." }

it "creates a reverse page link" do
subject

expect(reverse_link_finder.count).to eq(1)
end
end

context "when the semantic reference uses the ### widget syntax" do
let(:text) { "Detailed: ####{work_package.identifier}." }

it "creates a reverse page link" do
subject

expect(reverse_link_finder.count).to eq(1)
end
end

context "when the project has been renamed and a historical alias is referenced" do
let(:text) { "Historical: #OLD-#{work_package.sequence_number}." }

before do
WorkPackageSemanticAlias.create!(work_package:, identifier: "OLD-#{work_package.sequence_number}")
end

it "still creates a reverse page link" do
subject

expect(reverse_link_finder.count).to eq(1)
end
end

context "when no work package matches the semantic reference" do
let(:text) { "Missing: #GHOST-99." }

it "does not create a link" do
subject

expect(Wikis::ReverseInlinePageLink.count).to eq(0)
end
end

context "when the semantic identifier is followed by an alphanumeric word character" do
let(:text) { "Boundary: ##{work_package.identifier}abc." }

it "does not create a link" do
subject

expect(Wikis::ReverseInlinePageLink.count).to eq(0)
end
end
end

context "with a semantic-shape reference in classic mode",
with_flag: { semantic_work_package_ids: false },
with_settings: { work_packages_identifier: "classic" } do
let(:text) { "See #PROJ-1 for context." }

it "does not create a link" do
subject

expect(Wikis::ReverseInlinePageLink.count).to eq(0)
end
end
end
Loading