Accept semantic work-package identifiers in CKEditor text macros#22976
Accept semantic work-package identifiers in CKEditor text macros#22976akabiru wants to merge 38 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the text-formatting #N work package reference macro to render semantic identifiers (label and URL) when semantic work package IDs are enabled, while adding infrastructure to batch-load referenced work packages to avoid N+1 queries during rendering.
Changes:
- Add doc-level preload/cleanup hooks to
PatternMatcherFilterso matchers can warm/drop per-render caches. - Implement a per-render WorkPackage lookup in
ResourceLinksMatcherand use it in theWorkPackageslink handler to renderformatted_idlabels anddisplay_idhrefs. - Add specs covering classic vs semantic rendering, missing-WP fallback, and a query-limit regression check.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb | Adds coverage for semantic vs classic #N rendering + fallback and a query limit assertion. |
| lib/open_project/text_formatting/matchers/resource_links_matcher.rb | Introduces thread-local per-render WP preload cache + doc scan to collect referenced ids. |
| lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb | Switches #N link rendering to use preloaded WP’s formatted_id/display_id when available. |
| lib/open_project/text_formatting/filters/pattern_matcher_filter.rb | Adds optional matcher hooks (preload_for_doc / cleanup_after_doc) around per-node processing. |
Three changes from the rails-code-reviewer agent + the GitHub Copilot
reviewer:
1. Switch from `thread_mattr_accessor` to `RequestStore.store` for the
per-render WorkPackage lookup. Aligns with the existing `Cache`,
`Setting`, `CustomStyle`, and `WorkPackage#available_custom_field_key`
conventions in the codebase. Cleanup-on-request-end via
`RequestStore::Middleware` is defense in depth — the filter's explicit
save/restore is what actually scopes the cache to a single render.
2. Save/restore around the per-node loop, exposed via a single yielding
API on the matcher: `with_preloaded_resources(doc, context) { … }`.
The previous lookup is captured on entry and restored on exit, so a
nested `format_text` (custom-field formatter, recursive markdown
render, etc.) cannot clobber the outer render's lookup. The flat
`preload_for_doc`/`cleanup_after_doc` pair is replaced; the filter
recursively wraps the loop in each matcher's hook.
3. Wrap the lookup behind two class methods — `work_package_for(id)` for
the link handler, `with_preloaded_resources` for the filter — so the
RequestStore key stays a private constant. Future storage swaps don't
ripple to callers.
Plus the GitHub Copilot review's two findings:
- Skip the preload entirely in classic mode (`semantic_mode_active?`
guard). `display_id` and `formatted_id` collapse to the numeric form,
so the link handler renders the legacy shape from `wp_id` alone — no
DB load required, restoring pre-PR query-free behaviour. New spec
"classic mode is query-free" is the regression guard.
- `extract_work_package_id` now requires `prefix.nil?` so prefixed
matches like `version#3` and `message#12` don't contribute to the
preload set, mirroring `LinkHandlers::WorkPackages#applicable?`.
New specs:
- save/restore semantics: nested `with_preloaded_resources` calls
preserve the outer lookup and clear after the outer block exits.
- classic mode: `format_text("#1 #2 #3")` issues zero `work_packages`
SELECTs.
Refs #22976 (review)
c270f7e to
de7ebd4
Compare
Deploying openproject with ⚡ PullPreview
|
de7ebd4 to
37c3350
Compare
Three changes from the rails-code-reviewer agent + the GitHub Copilot
reviewer:
1. Switch from `thread_mattr_accessor` to `RequestStore.store` for the
per-render WorkPackage lookup. Aligns with the existing `Cache`,
`Setting`, `CustomStyle`, and `WorkPackage#available_custom_field_key`
conventions in the codebase. Cleanup-on-request-end via
`RequestStore::Middleware` is defense in depth — the filter's explicit
save/restore is what actually scopes the cache to a single render.
2. Save/restore around the per-node loop, exposed via a single yielding
API on the matcher: `with_preloaded_resources(doc, context) { … }`.
The previous lookup is captured on entry and restored on exit, so a
nested `format_text` (custom-field formatter, recursive markdown
render, etc.) cannot clobber the outer render's lookup. The flat
`preload_for_doc`/`cleanup_after_doc` pair is replaced; the filter
recursively wraps the loop in each matcher's hook.
3. Wrap the lookup behind two class methods — `work_package_for(id)` for
the link handler, `with_preloaded_resources` for the filter — so the
RequestStore key stays a private constant. Future storage swaps don't
ripple to callers.
Plus the GitHub Copilot review's two findings:
- Skip the preload entirely in classic mode (`semantic_mode_active?`
guard). `display_id` and `formatted_id` collapse to the numeric form,
so the link handler renders the legacy shape from `wp_id` alone — no
DB load required, restoring pre-PR query-free behaviour. New spec
"classic mode is query-free" is the regression guard.
- `extract_work_package_id` now requires `prefix.nil?` so prefixed
matches like `version#3` and `message#12` don't contribute to the
preload set, mirroring `LinkHandlers::WorkPackages#applicable?`.
New specs:
- save/restore semantics: nested `with_preloaded_resources` calls
preserve the outer lookup and clear after the outer block exits.
- classic mode: `format_text("#1 #2 #3")` issues zero `work_packages`
SELECTs.
Refs #22976 (review)
d27f126 to
1c22b61
Compare
|
Just commenting here to make you aware of a (relatively recently introduced) feature that also parses work package referencing macros: modules/wikis/app/services/wikis/concerns/update_reverse_inline_wiki_page_links.rb It's used to find places where a wiki page references a work package and creates a formal database link for that. |
|
Thanks Jan, much appreciated- I'll take a look at that 👍🏾 |
In semantic mode, plain `#N` references inside formatted text rendered `<a href="/work_packages/N">#N</a>`. The link should carry the human-readable identifier on both the label and the href, matching how the `##N` quickinfo macro already renders via Angular. Two pieces: 1. `PatternMatcherFilter` gains an opt-in pre/cleanup hook around the per-node loop. Matchers that own per-render lookup caches (e.g. a batched WP load) implement `preload_for_doc` and `cleanup_after_doc` to populate and drop them. 2. `ResourceLinksMatcher` implements those hooks: it scans every text node for `#N` matches, runs a single batched `WorkPackage.where(id: ids)`, and exposes the result via a thread-isolated class attribute. The `WorkPackages` link handler reads from it to choose `wp.formatted_id` for the label and `wp.display_id` for the href. Falls back to the legacy `#N` shape when the WP isn't loadable (deleted, out of scope, or no preload ran). Visibility filtering is intentionally not introduced — the matcher links regardless of viewer permissions on the referenced WP, preserving pre-existing behaviour. Out of scope for this ticket. Refs https://community.openproject.org/work_packages/74315
Three changes from the rails-code-reviewer agent + the GitHub Copilot
reviewer:
1. Switch from `thread_mattr_accessor` to `RequestStore.store` for the
per-render WorkPackage lookup. Aligns with the existing `Cache`,
`Setting`, `CustomStyle`, and `WorkPackage#available_custom_field_key`
conventions in the codebase. Cleanup-on-request-end via
`RequestStore::Middleware` is defense in depth — the filter's explicit
save/restore is what actually scopes the cache to a single render.
2. Save/restore around the per-node loop, exposed via a single yielding
API on the matcher: `with_preloaded_resources(doc, context) { … }`.
The previous lookup is captured on entry and restored on exit, so a
nested `format_text` (custom-field formatter, recursive markdown
render, etc.) cannot clobber the outer render's lookup. The flat
`preload_for_doc`/`cleanup_after_doc` pair is replaced; the filter
recursively wraps the loop in each matcher's hook.
3. Wrap the lookup behind two class methods — `work_package_for(id)` for
the link handler, `with_preloaded_resources` for the filter — so the
RequestStore key stays a private constant. Future storage swaps don't
ripple to callers.
Plus the GitHub Copilot review's two findings:
- Skip the preload entirely in classic mode (`semantic_mode_active?`
guard). `display_id` and `formatted_id` collapse to the numeric form,
so the link handler renders the legacy shape from `wp_id` alone — no
DB load required, restoring pre-PR query-free behaviour. New spec
"classic mode is query-free" is the regression guard.
- `extract_work_package_id` now requires `prefix.nil?` so prefixed
matches like `version#3` and `message#12` don't contribute to the
preload set, mirroring `LinkHandlers::WorkPackages#applicable?`.
New specs:
- save/restore semantics: nested `with_preloaded_resources` calls
preserve the outer lookup and clear after the outer block exits.
- classic mode: `format_text("#1 #2 #3")` issues zero `work_packages`
SELECTs.
Refs #22976 (review)
Address follow-up polish on PR 22976 review: - Extract `OpenProject::TextFormatting::PreformattedBlocks` (BLOCKS set + ancestor? helper) so PatternMatcherFilter and ResourceLinksMatcher share a single source of truth for the `<pre>`/`<code>` ancestry skip. - Lift `parse_match(match)` so `process_match` and `extract_work_package_id` consume the same regex group → semantic name mapping. - `with_preloaded_resources` captures `previous` unconditionally so the `ensure` block no longer needs a `defined?(previous)` guard. - Preload `WorkPackage.where(id: ids).select(:id, :identifier)` only — `display_id`/`formatted_id` don't read other columns. - N+1 spec switches from `have_a_query_limit(1)` to a SQL-notification subscriber filtered to `FROM "work_packages"` SELECTs, avoiding false positives from incidental Setting/User/Project queries.
The simple `to_i.to_s` round-trip check was previously a private predicate inside `FinderMethods`. The text-formatting layer needs the same predicate to decide whether a `#PROJ-1`-shaped match should be preloaded as a WP reference, but pulling it through `FinderMethods` would couple the macro parser to finder internals. Move the predicate up one level to the parent module — the same place `ID_ROUTE_CONSTRAINT` already lives — and have `FinderMethods` delegate. Single source of truth, no behaviour change.
`#1234` text macros render `formatted_id`/`display_id` already, but the *input* side still requires the numeric primary key. Authors typing or pasting `#PROJ-1` (or `##PROJ-1` / `###PROJ-1`) in a comment, WP description, or meeting body would see literal text rather than a resolved link. Three changes that move together: 1. `ResourceLinksMatcher.regexp` — the hash-separator branch now accepts either the numeric shape `\d+` or the semantic shape `[A-Z][A-Z0-9_]*-\d+`, mirroring `WorkPackage::SemanticIdentifier:: ID_ROUTE_CONSTRAINT`. The revision branch (`r\d+`) stays numeric-only via a separate alternation. `parse_match` is the single site that maps the new regex group indices to semantic field names; everything else flows from there. 2. `LinkHandlers::WorkPackages#call` — splits into a numeric path (preserving the leading-zero rejection from before) and a semantic path. Semantic-shape input only links when `semantic_mode_active?` is true; classic instances render literal text. Plain `#PROJ-N` requires a cache hit (literal-text fallback when missing); `##PROJ-N` / `###PROJ-N` quickinfo elements emit unconditionally with `data-id` set to the user-facing identifier — APIv3 already resolves either shape, and the frontend Angular component handles missing WPs. Hover-card URLs now also speak `display_id` so the URL matches the user-facing identifier (the route accepts both shapes — see `HoverCardComponent#initialize`). 3. The preload cache extends to string keys via the `WorkPackage.where_display_id_in` batch finder added in #23016. `with_preloaded_resources` runs one WP SELECT for the common case (numerics + current semantic identifiers); historical alias references add a second targeted alias-table pluck, so an alias-heavy doc costs at most two round-trips per render. Specs cover: `#PROJ-N` resolves with formatted_id / display_id href in semantic mode; classic mode leaves it as literal text and issues zero WP SELECTs; `##/###` quickinfo carries `display_id` in `data-id`; mixed numeric+semantic resolves in 1 SELECT; alias references resolve in 2 round-trips; `#GHOST-99` falls through cleanly; nested `format_text` calls preserve outer save/restore semantics.
`Macros::Links` extends `ResourceLinksMatcher`, so the regex broadening in the previous commit also matches `#PROJ-1` shapes inside markdown that's about to be exported as PDF. Without an explicit guard, the PDF custom handler would silently emit `<mention data-id="0">` (since `"PROJ-1".to_i == 0`) — broken-looking output that's hard to attribute. Tighten `WorkPackagesLinkHandler#applicable?` to reject semantic-shape input. Override `call` so it doesn't fall through the parent's cache-driven path (PDF rendering walks Markly nodes via a separate pipeline that doesn't populate the per-render cache). Cite WP #74366 in the comment as the follow-up that adds semantic-id support to the PDF side via the Markly walk in `app/models/exports/pdf/common/macro.rb`. New specs assert `#PROJ-1` falls through to literal text in PDF output and never produces a `data-id="0"` mention, both alone and mixed with numeric references.
`MentionFilter#work_package_mention` and `LinkHandlers::WorkPackages#render_work_package_macro` render `<opce-macro-wp-quickinfo data-id="<wp.id>" data-display-id="<wp.display_id>" data-detailed="…">`. `data-id` is the work-package id (stable across renames); `data-display-id` is the user-facing identifier (semantic in semantic mode, numeric string in classic). The convention matches the wire form on `<mention>` envelopes and the `data-type="user"`/`"group"` mention convention, where `data-id` has always been the record id. The link-handler fetches the work package on the semantic-mode quickinfo branch too — the preload is already populated in semantic mode, so this is a `RequestStore` hit, not a SELECT. `MentionFilter` reads `data-id` and resolves via `find_by(id:)`. Non-numeric `data-id` (parser-emitted source-typed mentions) falls back to literal text. `WorkPackageQuickinfoMacroComponent` reads `dataset.displayId ?? dataset.id` so stored markdown produced before the attribute split keeps loading: legacy `<opce-macro-wp-quickinfo data-id="DISPLAY">` resolves via the fallback; new shape resolves via the preferred attribute. Backend specs lock the new shape end-to-end: the link handler test fixture, the in-tool-links pipeline test, and the MentionFilter spec all assert distinct `data-id` (id) and `data-display-id` (display_id) values where they diverge, and identical values where they don't.
The `## / ###` autocomplete assertions pin both `data-id` (the record id, `mentioned_work_package.id` in either mode) and `data-display-id` (what the user typed — `wp_display_id` resolving to the numeric id in classic and the identifier in semantic) on the widget DOM that appears in the editor right after a pick. The classic / semantic asymmetry now lives in the assertion itself rather than in a per-context CSS selector.
Picks up the matched upstream work in opf/commonmark-ckeditor-build#113: - Parser recognises semantic identifiers (`#PROJ-N` / `##PROJ-N` / `###PROJ-N`) and guards stored mention envelopes against re-promotion on reload. - Mention feed and caster pass record id (`dataId`) and displayed identifier (`dataDisplayId`) through to the wire as `data-id` and `data-display-id`. The sidecar emission is gated to work-package mentions; user/group mentions stay single-attribute. - `##` / `###` autocomplete picks render as quickinfo widgets in both classic and semantic modes. Autocomplete persists as a `<mention>` envelope (id survives a rename or alias drop); source-typed shorthand persists as bare markdown. - Constantises the quickinfo model element name alongside the view tag for typo safety.
…isplayid-parity Use formatted_id and displayId in WP MentionFilter rendering
…playid-for-wp-text-macros
|
|
||
| # Unanchored shape of a semantic project identifier ("PROJ", "MY_PROJECT_1"). |
There was a problem hiding this comment.
Sure thing, I'll take a stab at that 👍🏾
| # PDF export only handles canonical numeric `#N` references. Semantic and | ||
| # leading-zero shapes fall through to literal text to avoid emitting a | ||
| # `<mention data-id="0">` (since `"PROJ-1".to_i == 0`). Semantic-id | ||
| # support in PDF export is tracked in https://community.openproject.org/wp/74766. | ||
| def applicable? |
|
Note I'm splitting this PR into smaller chunks to ease code review 🧑🏭 |
The [A-Z][A-Z0-9_]* shape that defines a semantic project identifier appears in three places that each redefine it inline: the validator's body check, the work-package semantic-id pattern composed into the route constraint, and the wiki reverse-link parser. Lift the unanchored shape to Projects::Identifier::SEMANTIC_FORMAT and have WorkPackage::SemanticIdentifier::SEMANTIC_ID_PATTERN compose from it via .source, with ID_ROUTE_CONSTRAINT in turn composing from that. The validator keeps its own anchored start/body patterns because each pattern produces a distinct error message — composing the two from SEMANTIC_FORMAT would obscure that contract. Split out of #22976 so #23186 can introduce its CLASSIC_IDENTIFIER_CHARS counterpart without conflicting with the larger feature PR.
Three changes from the rails-code-reviewer agent + the GitHub Copilot
reviewer:
1. Switch from `thread_mattr_accessor` to `RequestStore.store` for the
per-render WorkPackage lookup. Aligns with the existing `Cache`,
`Setting`, `CustomStyle`, and `WorkPackage#available_custom_field_key`
conventions in the codebase. Cleanup-on-request-end via
`RequestStore::Middleware` is defense in depth — the filter's explicit
save/restore is what actually scopes the cache to a single render.
2. Save/restore around the per-node loop, exposed via a single yielding
API on the matcher: `with_preloaded_resources(doc, context) { … }`.
The previous lookup is captured on entry and restored on exit, so a
nested `format_text` (custom-field formatter, recursive markdown
render, etc.) cannot clobber the outer render's lookup. The flat
`preload_for_doc`/`cleanup_after_doc` pair is replaced; the filter
recursively wraps the loop in each matcher's hook.
3. Wrap the lookup behind two class methods — `work_package_for(id)` for
the link handler, `with_preloaded_resources` for the filter — so the
RequestStore key stays a private constant. Future storage swaps don't
ripple to callers.
Plus the GitHub Copilot review's two findings:
- Skip the preload entirely in classic mode (`semantic_mode_active?`
guard). `display_id` and `formatted_id` collapse to the numeric form,
so the link handler renders the legacy shape from `wp_id` alone — no
DB load required, restoring pre-PR query-free behaviour. New spec
"classic mode is query-free" is the regression guard.
- `extract_work_package_id` now requires `prefix.nil?` so prefixed
matches like `version#3` and `message#12` don't contribute to the
preload set, mirroring `LinkHandlers::WorkPackages#applicable?`.
New specs:
- save/restore semantics: nested `with_preloaded_resources` calls
preserve the outer lookup and clear after the outer block exits.
- classic mode: `format_text("#1 #2 #3")` issues zero `work_packages`
SELECTs.
Refs #22976 (review)
Three changes from the rails-code-reviewer agent + the GitHub Copilot
reviewer:
1. Switch from `thread_mattr_accessor` to `RequestStore.store` for the
per-render WorkPackage lookup. Aligns with the existing `Cache`,
`Setting`, `CustomStyle`, and `WorkPackage#available_custom_field_key`
conventions in the codebase. Cleanup-on-request-end via
`RequestStore::Middleware` is defense in depth — the filter's explicit
save/restore is what actually scopes the cache to a single render.
2. Save/restore around the per-node loop, exposed via a single yielding
API on the matcher: `with_preloaded_resources(doc, context) { … }`.
The previous lookup is captured on entry and restored on exit, so a
nested `format_text` (custom-field formatter, recursive markdown
render, etc.) cannot clobber the outer render's lookup. The flat
`preload_for_doc`/`cleanup_after_doc` pair is replaced; the filter
recursively wraps the loop in each matcher's hook.
3. Wrap the lookup behind two class methods — `work_package_for(id)` for
the link handler, `with_preloaded_resources` for the filter — so the
RequestStore key stays a private constant. Future storage swaps don't
ripple to callers.
Plus the GitHub Copilot review's two findings:
- Skip the preload entirely in classic mode (`semantic_mode_active?`
guard). `display_id` and `formatted_id` collapse to the numeric form,
so the link handler renders the legacy shape from `wp_id` alone — no
DB load required, restoring pre-PR query-free behaviour. New spec
"classic mode is query-free" is the regression guard.
- `extract_work_package_id` now requires `prefix.nil?` so prefixed
matches like `version#3` and `message#12` don't contribute to the
preload set, mirroring `LinkHandlers::WorkPackages#applicable?`.
New specs:
- save/restore semantics: nested `with_preloaded_resources` calls
preserve the outer lookup and clear after the outer block exits.
- classic mode: `format_text("#1 #2 #3")` issues zero `work_packages`
SELECTs.
Refs #22976 (review)
Ticket
https://community.openproject.org/work_packages/74315
What are you trying to accomplish?
Make work-package text macros render project-based semantic identifiers in semantic mode. Authors typing or pasting
#PROJ-1,##PROJ-1,###PROJ-1(or picking a WP from the editor's autocomplete) get a resolved link or quickinfo. Hover-card URLs and the markdown source the editor saves both use the user-facing identifier. Numeric#1234keeps working in both modes — historical content in semantic mode, native form in classic.Note
Paired with commonmark-ckeditor-build PR 113; both ship together.
Out of scope:
<mention data-id="0">.Screenshots
Activity tab — automatic cause-formatter entries use the semantic identifier in both link label and href:
Rendered comment —
#KSTP-2,##KSTP-2,###KSTP-2triggers all carry the semantic identifier through the macro pipeline:Autocomplete dropdown labels show
Type SEMANTIC-ID: subject(driven by mode-awareWorkPackage#to_s):##quickinfo trigger — autocomplete then the inserted widget displays the semantic identifier:What approach did you choose and why?
The macro regex reuses the semantic-id pattern from the route constraint, so routing, formatting, and the editor's markdown processor share one source of truth. The hash and revision separators sit on independent alternation branches — the semantic shape applies only to
#references, revisions stay numeric.Resolution is mode-gated. Classic instances render semantic-shaped input as literal text (a
/work_packages/PROJ-1URL would 404 anyway). In semantic mode, plain#PROJ-1requires a resolvable WP — unresolvable references fall through to literal text rather than producing a broken link.##/###quickinfo elements emit unconditionally with the user-facing identifier indata-id, since the frontend handles missing WPs. Hover-card URLs use the same identifier so users see consistent paths everywhere.The per-render cache is keyed by the input string rather than by primary key, which makes the link handler symmetrical for numeric and semantic input. One batched query covers the common case in a single SELECT (numeric and current semantic identifiers); historical alias references — for projects that have been renamed — add one targeted alias-table lookup. At most two round-trips per render, no N+1.
On the CKEditor side,
data-idcarries the user-facing identifier end-to-end. No sidecar attribute, no precedence rules between attributes that could disagree. The Angular component that consumesdata-idalready passes it straight through to APIv3, which resolves either shape viafind_by_display_id— so the frontend never needs to know which mode the instance is in.Merge checklist
#PROJ-1, pick a WP from autocomplete, hover-card URL and href both use the semantic identifier, classic mode leaves#PROJ-1as literal text (screenshots above)#PROJ-1renders as literal text (nodata-id="0"mention)