bug/74762 Numeric ID in the email notification after adding watchers - #23337
Merged
akabiru merged 20 commits intoMay 28, 2026
Merged
Conversation
akabiru
changed the title
The `mentioned` and `watcher_changed` text-mailer bodies surfaced raw journal markdown — numeric `#42` references stayed numeric in semantic mode, and `<mention>` envelopes leaked as HTML source. Introduces `:plain_text` as a sibling format inside the existing Plain module. The filter chain mirrors the markdown pipeline (markdown, sanitization, mention, pattern-matcher) and finishes with a new `PlainTextOutputFilter` that collapses the DOM to text. The `WorkPackages` link handler and `MentionFilter` get plain-text branches keyed off `context[:plain_text]` so identifier resolution stays in one place across rich and plain channels. Closes https://community.openproject.org/wp/74762
The macro preload was visibility-scoped — references to work packages the recipient cannot see would fall through to the literal `#43` shape, even when the same reference rendered as `DCP-1` for an author with full view permission. Notification recipients saw misleading numeric ids for cross- project references in journal notes. Splits label resolution from link gating: - `ResourceLinksMatcher.build_lookup` now does an unscoped fetch for the primary identifier and a separate visibility-scoped id pluck. The link handler reads `visible_to_current_user?` to decide between a navigable anchor and a plain-text label. - `UpdateAncestorsService#set_journal_note` writes `#display_id` so new notes carry the semantic shape at the source; render-time resolution heals legacy `#N` content for users with view permission. Tradeoff: a recipient without view permission now sees the WP's semantic identifier (e.g. `DCP-1`) as plain text rather than `#43`. The reference's existence was already disclosed by the stored journal text; the project identifier is the only new piece of information surfaced, and is not treated as a secret elsewhere in the system (URLs, exports, API).
Pairs unscoped label resolution and viewer-scoped link gating in a WorkPackagePreloadCache instead of two RequestStore keys with a five-method save/restore protocol. Exposes one `current_cache` reader; consumers ask the cache directly via `fetch` and `visible?`. Extracts a `text_only?` predicate in the WP link handler so the `context[:plain_text]` and invisible-WP guards collapse into a single call site. `SemanticIdentifier.format` renames its parameter to reflect that the input may or may not be semantic.
The mention filter previously dropped to the envelope's stored text when the recipient lacked view permission on the referenced work package, which left stale identifiers in mailer bodies after a project rename and diverged from the `#N` text-reference path on the same render. Adopts the two-SELECT pattern ResourceLinksMatcher uses for `#N` references: a single unscoped batched lookup for label resolution plus a visibility-scoped id pluck for anchor gating. Invisible WPs render as plain text with the current formatted_id; the per-mention `WorkPackage.visible.find_by` is gone.
The format runs the full markdown pipeline and then collapses the DOM to text — it has nothing to do with the existing `:plain` format, which strips markdown entirely. Moves the formatter under the Markdown namespace next to the rich-output formatter whose pipeline it mirrors, and renames the symbol so the relationship is legible from the formatter_for case clause.
The `##N` and `###N` work-package macros emit JS-hydrated `<opce-macro-wp-quickinfo>` custom elements, which mail clients collapse to empty bullets. Introduce a `:markdown_as_static_html` format that shares the rich filter chain but signals `context[:as_static_html]` so the matcher and `MentionFilter` emit a server-rendered anchor — formatted_id, type name, subject, and (for `###`) status name — closely mirroring the in-app widget once flattened. Mailer HTML templates (`mentioned`, `watcher_changed`, `_work_package_details`) opt into the new format. Invisible WPs still render as plain-text labels, matching the cross-project visibility policy. `ResourceLinksMatcher.build_cache` and `MentionFilter#preload_work_package_mentions` eager-load `:type` and `:status` only when `:as_static_html` is set, leaving the default web path's two-SELECT shape untouched. Classic-mode preload now also runs under `:as_static_html` so the link handler can resolve type/subject for `##`/`###`. Renames the internal flag `context[:plain_text]` to `context[:as_text]` to restore symmetry with the user-facing `:markdown_as_text` format.
Reviewer feedback on cd122f8: the `parts << status / type / label` block was duplicated between the regex-driven (`WorkPackages` link handler) and envelope-driven (`MentionFilter`) static paths, with no guard against silent desynchronisation. Centralise the composition on the link handler and document why the two callers pass different labels — the regex path preserves the alias-as-matched, the envelope path normalises to the WP's current formatted_id.
The earlier write-time canonicalization stored the rendered
display_id ("#PROJ-7") in the journal note, which would rot under
project-identifier renames and leave dangling semantic strings if
semantic mode were rolled back. Restore the PK shape ("#42") and
let the formatter pipeline turn it into the user-facing identifier
at render time, where the resolver already handles both modes.
Both spec contexts now assert the same PK shape; the mode-specific
rendering of "#N" lives in the formatter specs.
Lift the static-anchor label composition out of LinkHandlers and into a small Helpers::StaticMacroLabel module so the envelope path (MentionFilter) and the text-reference path (LinkHandlers) share one shape — same module called from both, no cross-class reach-through. Batch the User and Group mention preloads alongside the existing WP preload so a note with N principals costs one SELECT per type rather than N. Class lookup now reads from indexed hashes; visibility-gating stays where it was (at the find for principals, separate from the label for WPs). Rename SemanticIdentifier.format → with_hash_prefix; the prior name was broad enough to invite misuse for arbitrary work-package values. Override StaticHtmlFormatter#filters explicitly so a future filter appearing in Formatter#filters is a deliberate decision to apply to mailer-side rendering, not an automatic one. Spec coverage: classic-mode quickinfo and inaccessible-WP paths (symmetric with the existing semantic-mode contexts), a principal preload N+1 guard, and an anonymous current_user smoke test that confirms the static-HTML pipeline doesn't raise when invoked without an authenticated viewer.
Compress the "do we need to load WP records?" condition into `preload_required?(context)` so the call site reads as intent rather than a tangle of two unrelated signals. The reasoning (semantic mode needs row lookup; static-HTML output needs type/subject for the anchor) moves to a comment on the predicate, where it belongs.
The feature flag is gone on release/17.5 (PR #23324); the `with_settings: { work_packages_identifier: ... }` annotation alone is enough to pin classic vs semantic behaviour in each context.
Strip explanatory paragraphs and cross-reference jargon
("Mirrors X", "Shared with Y", etc.) from comments introduced
on this branch. Keep only the WHY — the parts of the design
intent that aren't already evident from method names and
short bodies. Pre-existing comments on methods this branch
didn't author stay as-is.
akabiru
force-pushed
the
bug/74762-numeric-id-in-the-email-notification-after-adding-watchers
branch
from
May 26, 2026 11:34
09e2e6d to
2ca379f
Compare
The static-HTML pipeline differs from the rich pipeline only by a context flag - both share the same filter chain. The dedicated `Markdown::StaticHtmlFormatter` and `:markdown_as_static_html` format symbol were pure boilerplate around that one-line override. Callers now pass `format: :rich, static_html: true` and the matchers read `context[:static_html]` directly.
`:rich` is the default at every layer of the `format_text` chain (view helper → `OpenProject::TextFormatting#format_text` → `Renderer.format_text`), so the explicit keyword adds noise.
Same pattern as the static-HTML collapse: the `markdown_as_text` format symbol was a thin subclass setting a context flag and swapping the filter list. Replace it with `plain_text: true` on the existing rich formatter, which now picks between `RICH_FILTERS` and `TEXT_FILTERS` constants based on the flag. `static_html:` and `plain_text:` now sit as peer options on one format. Rename the `as_text` context key to `plain_text` for symmetry with `static_html`. Update both mailer `.text.erb` views and the two handler predicates that branch on the flag.
akabiru
force-pushed
the
bug/74762-numeric-id-in-the-email-notification-after-adding-watchers
branch
from
May 26, 2026 19:10
c273cd6 to
f5957d8
Compare
akabiru
marked this pull request as ready for review
May 26, 2026 19:10
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes work package reference rendering in watcher-changed and mentioned notification emails so that both HTML and plain-text bodies consistently render references in the configured identifier mode (semantic vs classic), and provides a static-HTML fallback for quickinfo macros that can’t be hydrated by mail clients.
Changes:
- Route mailer rendering through the shared text-formatting pipeline with two output modes:
static_htmlfor HTML emails andplain_textfor text emails. - Rework work package preloading to split unscoped label resolution from viewer-scoped link gating, preventing links/hovercards for inaccessible references.
- Add/extend specs around identifier formatting, markdown output modes, mention handling, and query bounds.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/services/work_packages/update_ancestors_service_spec.rb | Adds coverage ensuring auto-generated journal notes store PK-based references regardless of identifier mode. |
| spec/models/work_package/semantic_identifier_spec.rb | Adds unit tests for format_display_id formatting behavior. |
| spec/mailers/work_package_mailer_spec.rb | Adds mailer coverage for referenced WP rendering in mentioned/watcher-changed emails. |
| spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb | Updates cache expectations and adjusts query-bound assertions for the new preload strategy. |
| spec/lib/open_project/text_formatting/formats/markdown/text_rendering_spec.rb | New specs validating plain-text markdown rendering (no HTML leakage, correct WP labels). |
| spec/lib/open_project/text_formatting/formats/markdown/static_html_rendering_spec.rb | New specs validating static-HTML rendering for quickinfo macros and inaccessible WPs. |
| spec/lib/open_project/text_formatting/filters/mention_filter_spec.rb | Extends mention specs for plain-text mode, inaccessible WPs, and principal preload batching. |
| lib/open_project/text_formatting/matchers/resource_links_matcher.rb | Introduces a preload cache object and implements unscoped lookup + visibility-gated link emission. |
| lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb | Uses the new preload cache and adds static-HTML macro rendering + text-only gating. |
| lib/open_project/text_formatting/helpers/static_macro_label.rb | New helper to build static anchor labels for WP quickinfo macros in non-hydrated channels. |
| lib/open_project/text_formatting/formats/markdown/formatter.rb | Splits markdown filters into rich vs plain-text pipelines and avoids html_safe for plain-text output. |
| lib/open_project/text_formatting/filters/plain_text_output_filter.rb | New terminal filter collapsing the DOM to visible text for text/plain output. |
| lib/open_project/text_formatting/filters/mention_filter.rb | Adds mention preloading, static-HTML quickinfo fallback, and plain-text/visibility gating. |
| app/views/work_package_mailer/watcher_changed.text.erb | Formats “latest note” through the plain-text pipeline to resolve references correctly. |
| app/views/work_package_mailer/watcher_changed.html.erb | Enables static_html rendering to avoid custom elements in email HTML. |
| app/views/work_package_mailer/mentioned.text.erb | Replaces strip_tags with plain-text pipeline rendering for journal notes. |
| app/views/work_package_mailer/mentioned.html.erb | Enables static_html rendering for journal notes in mentioned emails. |
| app/views/work_package_mailer/_work_package_details.html.erb | Renders description with static_html to avoid JS-hydrated elements in email HTML. |
| app/services/work_packages/update_ancestors_service.rb | Refactors journal note assignment formatting (no behavior change intended). |
| app/models/work_package/semantic_identifier.rb | Extracts display-id formatting into format_display_id and reuses it in formatted_id. |
akabiru
commented
May 26, 2026
| work_packages.each do |wp| | ||
| wp.journal_notes = I18n.t("work_package.updated_automatically_by_child_changes", child: "##{initiator_work_package.id}") | ||
| wp.journal_notes = I18n.t("work_package.updated_automatically_by_child_changes", | ||
| child: "##{initiator_work_package.id}") |
Member
Author
There was a problem hiding this comment.
ℹ️ deliberately left as a primary key for consistent translation (render) in semanic/classic mode
oliverguenther
requested changes
May 27, 2026
oliverguenther
left a comment
oliverguenther
left a comment
Member
There was a problem hiding this comment.
The changes look good to me overall. What I did notice:
- The change should be applied to all mailers calling format_text. Please introduce a mail-specific helper that always passes only_path: false and static_html: true, as it will be very easy to forget this, so that developers can simply call format_text in the mailer and be assured that it does what we expect it to
- Since only_path is only required when rendering outside of the application, both these flags could be unified into one (something along the lines of render_mode: :external(_html)
thykel
self-requested a review
May 27, 2026 09:24
`format_text` accepts `render_mode:` (`:in_app_html`, `:external_html`, `:external_text`), which resolves the `only_path`, `static_html` and `plain_text` context flags as a set. External surfaces (mailer HTML body, future RSS/PDF/webhook) need absolute URLs and static rendering together; pinning the trio at the public API keeps callers from forgetting one. Explicit primitive kwargs still override. `MailFormattingHelper` exposes `format_mail_html` and `format_mail_text` thin wrappers around `format_text(render_mode:)`. The `_html` / `_text` suffix matches the `.html.erb` / `.text.erb` template extension so caller intent stays visible in the view, with no introspection of `formats`. The five WorkPackageMailer view sites use the helpers; `_work_package_details`, `mentioned.html`, `mentioned.text`, `watcher_changed.html`, `watcher_changed.text` drop the `static_html:`/`only_path:`/`plain_text:` boilerplate.
Strip a forward-looking aside about future external surfaces in RenderMode; the invariant is that external surfaces need both absolute URLs and static rendering. Replace "in practice" with "a coupled set" to drop the soft hedge. Drop the lead "wrappers around format_text" sentence on MailFormattingHelper since the module body already shows the wrapping; the WHY (channel pinning, extension/helper name parity) is the part worth documenting.
This was referenced May 27, 2026
Member
Author
|
Thanks for the review @oliverguenther 🙌🏾 addressed in 499d782 and 3036e85 -> wider migration will follow separately against "dev" with a proposed custom cop. |
oliverguenther
approved these changes
May 28, 2026
akabiru
deleted the
bug/74762-numeric-id-in-the-email-notification-after-adding-watchers
branch
May 28, 2026 08:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
https://community.openproject.org/wp/74762
What are you trying to accomplish?
Watcher-changed and mentioned notification emails now render work-package references in the configured identifier mode (
DEMO-1in semantic mode,#42in classic) across both HTML and plain-text bodies. Previously the text body leaked raw<mention>HTML and the HTML body showed the numeric primary key regardless of mode.Quickinfo macros (
##42,###42) render as a single server-side anchor carrying type, status, and subject — mail clients don't execute the custom element used in the in-app preview, so the mailer needs a static fallback. Cross-project references the recipient can't see collapse to a plain-text identifier — no link to a 404, no subject leak.Screenshots
💌 HTML Email
📧 Plain Text email
What approach did you choose and why?
Mailer bodies now go through the same text-formatting pipeline as the in-app preview. Two context options shape its terminal output: one for server-rendered anchors (HTML email), one for plain-text bodies. One pipeline, two output modes. Work-package lookups are batched per fragment rather than fired per mention.
#DP-15<a>DP-15</a><a>#20</a>##DP-15<a>Task DP-15: Cats V Dogs</a><a>Task #20: Cats V Dogs</a>###DP-15<a>New Task DP-15: …</a><a>New Task #20: …</a>Inaccessible cross-project references collapse to plain text — no link, no subject leak:
##SECRET-1SECRET-1###SECRET-1SECRET-1Merge checklist