From 2ca379fe375c1e1ec7a136e580a768ee3310bc70 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 May 2026 14:32:37 +0300 Subject: [PATCH] Trim verbose comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../text_formatting/filters/mention_filter.rb | 37 ++++---------- .../filters/plain_text_output_filter.rb | 6 +-- .../formats/markdown/static_html_formatter.rb | 11 ++--- .../formats/markdown/text_formatter.rb | 8 ++-- .../helpers/static_macro_label.rb | 13 ++--- .../matchers/link_handlers/work_packages.rb | 17 ++----- .../matchers/resource_links_matcher.rb | 48 +++++-------------- 7 files changed, 38 insertions(+), 102 deletions(-) diff --git a/lib/open_project/text_formatting/filters/mention_filter.rb b/lib/open_project/text_formatting/filters/mention_filter.rb index 9ddef4e1285..2b85decfc00 100644 --- a/lib/open_project/text_formatting/filters/mention_filter.rb +++ b/lib/open_project/text_formatting/filters/mention_filter.rb @@ -49,13 +49,11 @@ module OpenProject::TextFormatting private - # Doc-level preload so a note with N user/group/WP mentions costs - # a bounded number of SELECTs rather than one per mention. WP - # mentions need the unscoped record (label) plus a separate - # visibility id pluck (anchor-vs-text gating); principals fold the - # two concerns into a single visibility-scoped fetch since - # invisible principals have no plain-text render path — they fall - # back to the literal envelope text instead. + # WP labels resolve regardless of viewer (so an inaccessible WP + # still renders its current formatted_id); a separate id pluck + # gates anchor-vs-text. Principals collapse the two concerns into + # one visibility-scoped fetch — invisible users and groups fall + # back to the literal envelope text. def preload_mentions preload_work_package_mentions preload_principal_mentions @@ -70,8 +68,6 @@ module OpenProject::TextFormatting end scope = WorkPackage.where(id: ids) - # Static-HTML channels need `type` and `status` to render - # quickinfo envelopes as anchors instead of `` widgets. scope = scope.includes(:type, :status) if context[:as_static_html] @mentioned_work_packages = scope.index_by(&:id) @visible_mentioned_ids = WorkPackage.visible.where(id: ids).pluck(:id).to_set @@ -118,8 +114,6 @@ module OpenProject::TextFormatting def work_package_mention(work_package, mention) return Nokogiri::XML::Text.new(work_package.formatted_id, mention.document) if text_only?(work_package) - # Match the label and URL convention used for `#N` text references - # elsewhere in the markdown pipeline. case mention.text.count("#") when 3 then work_package_quickinfo(work_package, detailed: true) when 2 then work_package_quickinfo(work_package, detailed: false) @@ -127,12 +121,8 @@ module OpenProject::TextFormatting end end - # Plain-text channels and inaccessible WPs both render the - # `formatted_id` without an anchor or quickinfo widget — the - # latter would resolve to a hover-card endpoint the recipient - # can't reach. Mirrors `LinkHandlers::WorkPackages#text_only?`, - # which gates the matching decision for `#N` text references; keep - # the two in sync. + # The hover-card endpoint a quickinfo would link to is unreachable + # for plain-text recipients and for viewers without view permission. def text_only?(work_package) context[:as_text] || @visible_mentioned_ids.exclude?(work_package.id) end @@ -147,11 +137,8 @@ module OpenProject::TextFormatting detailed: } end - # Static fallback shared with the PatternMatcherFilter's `##`/`###` - # path so envelope-driven and text-driven references render the same - # shape in channels that cannot hydrate the custom element. Always - # uses the WP's current `formatted_id`, normalising any historical - # alias the envelope may have been authored with. + # Uses the WP's current `formatted_id` rather than the envelope text, + # so a renamed identifier doesn't leave a stale label in the mailer. def work_package_static_macro(work_package, detailed:) label = OpenProject::TextFormatting::Helpers::StaticMacroLabel .call(work_package, label: work_package.formatted_id, detailed:) @@ -172,12 +159,6 @@ module OpenProject::TextFormatting }) end - # WP label resolution is unscoped (preloaded above); visibility is - # gated separately in `text_only?` so an inaccessible WP renders - # its current formatted_id rather than the envelope text the - # author originally typed. Principals stay visibility-gated at the - # preload — there's no equivalent text-only render path for users - # or groups, so invisible principals fall back to the literal text. def class_from_mention(mention) id = mention_id(mention)&.to_i case mention.attributes["data-type"].value diff --git a/lib/open_project/text_formatting/filters/plain_text_output_filter.rb b/lib/open_project/text_formatting/filters/plain_text_output_filter.rb index b23c992af0e..32873ddcb20 100644 --- a/lib/open_project/text_formatting/filters/plain_text_output_filter.rb +++ b/lib/open_project/text_formatting/filters/plain_text_output_filter.rb @@ -30,10 +30,8 @@ module OpenProject::TextFormatting module Filters - # Final stage of the plain-text pipeline. Earlier filters resolve - # mentions and macros to their text-mode shapes (driven by - # `context[:as_text]`); this stage collapses any remaining markup - # so the pipeline output is suitable for `text/plain` bodies. + # Final stage of the `markdown_as_text` pipeline — strips remaining + # markup so the output is safe for `text/plain` bodies. class PlainTextOutputFilter < HTML::Pipeline::Filter def call doc.text diff --git a/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb b/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb index dcf25ca8b5a..0cca7df54f8 100644 --- a/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb +++ b/lib/open_project/text_formatting/formats/markdown/static_html_formatter.rb @@ -30,13 +30,10 @@ module OpenProject::TextFormatting::Formats module Markdown - # Static-HTML sibling of `Markdown::Formatter`. Shares the same filter - # chain so identifier resolution, mention handling, and link rendering - # stay consistent, but signals `context[:as_static_html]` so matchers - # and filters emit server-rendered anchors in place of JS-hydrated - # custom elements. Intended for channels that cannot run JS — HTML - # mailers, server-side previews, archival exports — where dynamic - # widgets would collapse to empty placeholders. + # Inherits `Markdown::Formatter`'s filter chain but signals + # `context[:as_static_html]` so matchers emit server-rendered + # anchors instead of JS-hydrated custom elements. For channels + # that can't run JS — HTML mailers, server-side previews. class StaticHtmlFormatter < Formatter def initialize(context) super(context.merge(as_static_html: true)) diff --git a/lib/open_project/text_formatting/formats/markdown/text_formatter.rb b/lib/open_project/text_formatting/formats/markdown/text_formatter.rb index b320fb967e3..4f0430ed630 100644 --- a/lib/open_project/text_formatting/formats/markdown/text_formatter.rb +++ b/lib/open_project/text_formatting/formats/markdown/text_formatter.rb @@ -30,11 +30,9 @@ module OpenProject::TextFormatting::Formats module Markdown - # Text-output sibling of `Markdown::Formatter`. Shares the matcher and - # mention pipeline with the rich renderer so identifier resolution - # stays consistent across channels, then collapses the final DOM to - # text via `PlainTextOutputFilter`. Intended for plain/text mailers - # and other channels where HTML would be a foreign body. + # Runs the matcher and mention pipeline, then collapses the DOM to + # text via `PlainTextOutputFilter`. For `text/plain` mailers and + # other channels where HTML would be a foreign body. class TextFormatter < OpenProject::TextFormatting::Formats::BaseFormatter def initialize(context) super(context.merge(as_text: true)) diff --git a/lib/open_project/text_formatting/helpers/static_macro_label.rb b/lib/open_project/text_formatting/helpers/static_macro_label.rb index b67b8b0fdce..2c33e269044 100644 --- a/lib/open_project/text_formatting/helpers/static_macro_label.rb +++ b/lib/open_project/text_formatting/helpers/static_macro_label.rb @@ -30,16 +30,9 @@ module OpenProject::TextFormatting module Helpers - # Composes the anchor text for a work-package quickinfo macro rendered - # in a static-HTML channel (mailers, server-side previews) — channels - # that cannot hydrate the JS-driven `` widget - # and so flatten the macro to an `` whose text must carry enough - # context for a reader to recognise the reference: type, optional - # status, the identifier label, and the subject. - # - # Shared between the text-reference path (`LinkHandlers::WorkPackages`) - # and the envelope path (`Filters::MentionFilter`) so both render the - # same shape for the same WP. + # Anchor text for the static-HTML form of a WP quickinfo macro: + # `[status ]type label: subject`. Used in channels (HTML mailers, + # server-side previews) that can't hydrate the `` widget. module StaticMacroLabel def self.call(work_package, label:, detailed:) parts = [] 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 60039848feb..c6c27c2369d 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 @@ -111,13 +111,8 @@ module OpenProject::TextFormatting::Matchers data: { id:, display_id:, detailed: } end - # Static fallback for channels that cannot hydrate the quickinfo - # custom element. Composes type, optional status, label, and subject - # into the anchor; unresolved references collapse to the bare label. - # The label is the matched identifier (potentially a historical alias) - # to preserve what the author wrote — the `` envelope path - # in `MentionFilter` instead normalises to the WP's current - # `formatted_id`. + # The label keeps what the author wrote (possibly a historical + # alias) so the rendered text matches the source markdown. def render_static_work_package_macro(work_package, label, detailed:) return label unless work_package @@ -144,12 +139,8 @@ module OpenProject::TextFormatting::Matchers }) end - # Plain-text channels and inaccessible WPs both render the label - # without an anchor or quickinfo. Visibility is checked only when a - # WP was preloaded — a nil work_package means a classic-mode render - # or an unresolved reference, neither of which needs gating. Mirrors - # `MentionFilter#text_only?`, which gates the same decision for - # `` envelopes; keep the two in sync. + # A nil WP means classic mode skipped the preload, or the reference + # didn't resolve — neither case needs visibility gating. def text_only?(work_package) context[:as_text] || (work_package && !preload_cache.visible?(work_package.id)) 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 772b98a4333..ae82e8669c6 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -69,11 +69,9 @@ module OpenProject::TextFormatting # identifier:version:1.0.0 # identifier:source:some/file class ResourceLinksMatcher < RegexMatcher - # Per-render preload state shared between the matcher and the link - # handler. Pairs unscoped label resolution (`lookup`) with viewer-scoped - # link gating (`visible_ids`) so a single render computes both with a - # bounded number of round-trips and renders consistent labels for - # invisible WPs. + # Unscoped label resolution (`lookup`) paired with viewer-scoped + # link gating (`visible_ids`), so the link handler renders the + # same label for everyone and decides anchor-vs-text per viewer. class WorkPackagePreloadCache attr_reader :lookup, :visible_ids @@ -90,8 +88,7 @@ module OpenProject::TextFormatting visible_ids.include?(work_package_id) end - # Per-process frozen singleton returned when no preload is in - # scope — not a factory; callers must not mutate it. + # Frozen singleton, not a factory — callers must not mutate it. EMPTY = new(lookup: {}.freeze, visible_ids: Set.new.freeze).freeze end @@ -156,16 +153,13 @@ module OpenProject::TextFormatting ] end - # The active preload cache for the current render, or an empty - # singleton when no preload is in scope (classic mode, no `#N` - # references, or rendering outside `with_preloaded_resources`). def self.current_cache RequestStore.store[CACHE_KEY] || WorkPackagePreloadCache::EMPTY end - # Doc-level preload called by `PatternMatcherFilter`. Save/restores - # the cache so a nested `format_text` (e.g. custom-field formatter - # re-entering the pipeline) doesn't clobber the outer render. + # Save/restore so a nested `format_text` (e.g. a custom-field + # formatter re-entering the pipeline) doesn't clobber the outer + # render's cache. def self.with_preloaded_resources(doc, context) previous = RequestStore.store[CACHE_KEY] return yield unless preload_required?(context) @@ -179,12 +173,8 @@ module OpenProject::TextFormatting RequestStore.store[CACHE_KEY] = previous end - # Two channels need the WP record at render time: semantic mode (to - # resolve `PROJ-7` to a row) and static-HTML output (to compose the - # type/subject anchor of a quickinfo macro). Classic-mode rich HTML - # and the as-text channel both render from the matched id alone — - # the latter short-circuits on `text_only?` before consulting the - # cache. + # Semantic mode needs the row to map `PROJ-7` to an id; static-HTML + # output needs `type`/`subject` to compose the quickinfo anchor. def self.preload_required?(context) Setting::WorkPackageIdentifier.semantic? || context[:as_static_html] end @@ -215,22 +205,10 @@ module OpenProject::TextFormatting identifier end - # Label resolution is unscoped: a reference to an inaccessible - # work package still renders as its semantic identifier (e.g. - # `DCP-1`), just without a navigable anchor. Visibility is - # captured separately so the link handler can pick anchor-vs-text - # per WP without re-querying. - # - # Two SELECTs in the common case: one unscoped fetch by identifier, - # one visibility-scoped id pluck. A third targeted SELECT fires - # for historical aliases — the loaded row carries only the current - # identifier, so unmapped inputs are filled in from - # `WorkPackageSemanticAlias`. Static-HTML channels also eager-load - # `:type` and `:status` so the link handler can render the - # static-anchor variant of `##`/`###` macros without N+1 queries — - # those associations are the metadata a reader needs to recognise a - # WP reference flattened to text. Anything beyond that (project, - # versions, custom fields) stays out of this preload. + # Two SELECTs in the common case (unscoped fetch + visibility id + # pluck), a third when historical aliases need resolving. Static- + # HTML output additionally needs `:type` and `:status` to compose + # the anchor for `##`/`###` macros. def self.build_cache(identifiers, context = {}) scope = WorkPackage.where_display_id_in(*identifiers) scope = if context[:as_static_html]