Editor round-trip - semantic-id-aware mention contract#23204
Open
akabiru wants to merge 10 commits into
Open
Conversation
akabiru
force-pushed
the
feature/text-macro-semantic-id-rendering
branch
from
f83491a to
c365ceb
Compare
akabiru
force-pushed
the
feature/editor-mention-semantic-id
branch
from
c775cff to
7112d77
Compare
akabiru
force-pushed
the
feature/text-macro-semantic-id-rendering
branch
from
76dc2e2 to
5feb9d3
Compare
akabiru
force-pushed
the
feature/editor-mention-semantic-id
branch
from
810b665 to
f2d3302
Compare
akabiru
force-pushed
the
feature/text-macro-semantic-id-rendering
branch
from
66da22d to
cbeaeca
Compare
4 tasks
`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 CKEditor mention plugin (in commonmark-ckeditor-build) reads `displayId` from each search result so the markdown source it inserts on autocomplete-pick speaks the user-facing identifier — `#PROJ-7` in semantic mode, `#1234` in classic. The endpoint previously serialised `work_package.attributes`, which gave the raw `identifier` column but not the mode-aware `display_id` / `formatted_id` accessors. Fold both into the JSON response. `displayId` collapses to the numeric id in classic mode, so the frontend never needs a per-mode branch.
The only consumer of `/work_packages/auto_complete.json` is the CKEditor mention feed in commonmark-ckeditor-build, which reads `displayId` (needed to build the markdown source `#PROJ-7` and the link URL) and `to_s` (used as the dropdown label). `formattedId` was added alongside `displayId` for symmetry but nothing reads it.
The auto_complete endpoint exposed `displayId` as the bare `work_package.display_id`, which is an Integer in classic mode and a String in semantic mode. APIv3 already coerces this field on the work package representer (and on its `:children` and `:ancestors` link collections), so the auto_complete payload was the lone outlier. The single CKEditor consumer interpolates the value into a string, so this change is purely a consistency fix at the JSON boundary; nothing behavioural depends on the wire type.
The CKEditor mention plugin reads `displayId` (camelCase, stringified) off each entry to insert `#PROJ-7` or `#1234` into the editor source and to build the mention's link URL. The contract must hold in both classic and semantic mode so the frontend doesn't have to branch on identifier shape, and the comment in the controller cites APIv3 parity as the reason the value is forced to a String. Render-views spec asserts the camelCase key, the stringified value, and the mode-conditional shape (numeric id in classic, semantic identifier in semantic).
The "can autocomplete work packages with different triggers" example exercised \`#\` / \`##\` / \`###\` markers but only in classic mode with hardcoded numeric ids — the whole point of the PR is mode-aware behaviour, so semantic mode getting no integration coverage was a real gap. Wrap the body as a \`shared_examples\` and pull in via two contexts. Parameterised over \`wp.display_id\` (what the user types) and \`wp.formatted_id\` (the visible link label in rendered journal notes). In classic mode both collapse to the existing strings, so the original assertions still hold; in semantic mode they expand to \`PROJ-7\`.
The previous version reached for `wp.display_id` and `wp.formatted_id`
inside the example body, so the spec was implicitly re-testing the
dispatch logic of those helpers (covered elsewhere) rather than
asserting on the column values it actually cares about.
Each consuming context now defines `wp_id` and `wp_label` lets so the
shared example asserts directly on the typed value and the rendered
link label. Classic mode passes `wp.id` and `"##{wp.id}"`. Semantic
mode passes `wp.identifier` for both — the rendered link drops the `#`
for semantic identifiers.
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.
- mention_filter.rb: rename local `href_id` to `display_id` to mirror the method name and the `data-display-id` wire attribute (the value is the user-facing identifier, used for both the href and the data-attribute). - auto_completes_controller.rb: trim the `displayId` doc comment that incorrectly claimed the editor builds the mention's link URL from this field. The URL is composed server-side at render time from `data-id` / `data-display-id`; the editor only inserts the markdown source.
akabiru
force-pushed
the
feature/editor-mention-semantic-id
branch
from
f2d3302 to
27c6f72
Compare
akabiru
changed the base branch from
feature/text-macro-semantic-id-rendering
to
feature/text-macro-preload-cache
Deploying openproject with ⚡ PullPreview
|
akabiru
changed the title
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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/74949
Note
Stacked on #23203. Final slice carved out of #22976. Paired with commonmark-ckeditor-build PR 113; both ship together per the build repo's README.
What are you trying to accomplish?
Close the editor round-trip for work-package mentions: an author picks
#PROJ-7from autocomplete and the markdown source persists as<mention data-id="10244" data-display-id="PROJ-7" …>. The reverse direction — stored<mention data-id="10244" data-display-id="PROJ-7">renders as a link whose label and href both output the user-facing identifier. With this slice plus CKEditor PR 113 merged, the contract is consistent end-to-end across both modes.Contract
data-id= work-package id (stable across renames).data-display-id= user-facing identifier (PROJ-7in semantic mode, the numeric id as a string in classic). Same convention on<mention>envelopes and<opce-macro-wp-quickinfo>. Matchesdata-type="user"/"group"mentions, which already used numericdata-id.Storage rule (CKEditor side)
One invariant across both modes:
<mention>envelope.The presence of a record-id signal (
wpIdon the widget model,data-display-idon the wire) is the discriminator. Autocomplete picks set it; source-typed parses don't.Backend render matrix
<mention data-id="<id>" data-display-id="<displayId>" data-type="work_package">…</mention>data-display-id<mention data-id="42">(nodata-display-id)#42#N/#PROJ-N(source-typed)##N/###N(source-typed)<opce-macro-wp-quickinfo>widget (slice S4)auto_completereturnsdisplayIdas a string (APIv3 parity); the frontend treats it as the user-facing label and the value the macro pipeline can resolve.MentionFilterreadsdata-idto fetch the WP and renders the link withdisplay_idso the rendered output never disagrees with the envelope. The Angular fallback inwork-package-quickinfo-macro.component.tsprefersdataset.displayIdand falls back todataset.idso legacy stored markdown (single-attribute envelopes emitted before the split) keeps loading.Screenshots
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 split between
data-id(record id) anddata-display-id(user-facing identifier) means the envelope survives a rename: a stored mention keeps pointing at the same work package even after its project moves or the work package's identifier changes. The rendered label and href always resolve via the currentdisplay_id, so users see consistent text everywhere the macro pipeline runs.On the autocomplete API,
displayIdships as a string so it's interchangeable with the value the user typed and with what APIv3 already returns elsewhere.formattedIdwas briefly considered as a parallel field but folded back out —displayIdplus the front-end's existing label composition is sufficient. The mention-trigger feature spec runs in both classic and semantic mode through a shared example, which catches drift on either branch.The CKEditor bundle is rebuilt against PR 113's source so the OpenProject worktree carries the matching wire format. The rebuild commit is separated from the logic changes to keep the diff scannable.
Merge checklist
auto_completes_controller_spec,mention_filter_spec,in_tool_links_spec,link_handlers/work_packages_spec#PROJ-1/##PROJ-1/###PROJ-1, pick from autocomplete, stored markdown carries<mention data-id data-display-id>, reload and resave clean