Conversation
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
704c960 to
b29661a
Compare
Multi-value attributes such as target versions need both block and inline renderings depending on the surrounding text; an optional singleline/multiline macro argument lets authors choose.
The macro element's layout argument flows through displayFieldOptions; a singleline option selects a comma-joined display field instead of the one-per-line variants used in the single view context.
Mirrors the web matcher grammar and threads the layout to the export formatter's array separator. Multi-value macros now default to one value per line (markdown hard break) instead of a comma-joined list, matching the web rendering.
workPackageValue:X:version now renders all target versions of the work package, single-line by default so legacy macros keep their inline shape within existing content. Applies to both the web and export render paths.
b29661a to
efe27bf
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
The layout argument is the feature; defaults stay what each surface always rendered. Only an explicit multiline layout switches exports to one value per line, which also makes the legacy version layout pin redundant on this path.
A bare layout keyword directly after an id collapses to the relative reference form; quoting the keyword keeps it as an attribute name. Both sides of that contract are now guarded against regex changes.
The display field service spec locks the multi-value type list to the per-type multiline branches it mirrors, so adding a type to one side without the other fails a test. The macro component spec pins the deprecated version attribute mapping, including the explicit multiline override and the work-package-only gate.
reinterpret_as_relative_embed sits next to relative_embed?, which its result feeds, and returns the corrected hash instead of mutating its argument in place. Also threads layout as a keyword through format_attribute_value like the rest of the call chain.
valueString joins like the sibling multi-line fields so the title getter returns a string, and the singleline routing branch carries the same single-view container gate as the branches it sits beside. Spec now also covers the empty value list.
The undefined state stays meaningful (no author choice), the type now says so and rules out arbitrary strings.
The export resolver takes semantic references via find_by_display_id; these cases pin that the layout argument and the legacy version mapping hold when the work package is referenced by semantic identifier.
There was a problem hiding this comment.
Pull request overview
Adds an optional trailing layout argument (singleline / multiline) to attribute value macros so authors can control whether multi-value attributes render inline (comma-separated) or stacked (one per line). This is applied consistently across the markdown/web macro parsing, frontend rendering, and PDF/export macro path, including mapping legacy version to targetVersions with single-line default.
Changes:
- Extend attribute macro parsing to recognize a reserved
:singleline/:multilinelayout segment (including relative/id-less forms) and expose it viadata-layout. - Add frontend rendering support for the
singlelinelayout by routing relevant multi-value field types to a newSingleLineResourcesDisplayField. - Apply the same layout option to the PDF/export macro formatter and map legacy
versiontotarget_versions/targetVersionsbehavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/models/exports/pdf/common/macro_spec.rb | Adds PDF/export specs covering layout behavior for multi-value custom fields and target versions (incl. legacy version). |
| spec/lib/open_project/text_formatting/markdown/attribute_macros_spec.rb | Verifies markdown parsing emits data-layout correctly and handles relative/id-less forms. |
| lib/open_project/text_formatting/matchers/attribute_macros.rb | Extends the core attribute macro regex + extraction to parse and propagate the layout argument. |
| frontend/src/app/shared/components/fields/macros/attribute-value-macro.html | Passes the parsed layout into display field rendering options. |
| frontend/src/app/shared/components/fields/macros/attribute-value-macro.component.ts | Reads data-layout, maps legacy version → targetVersions, and defaults legacy layout to single-line. |
| frontend/src/app/shared/components/fields/macros/attribute-value-macro.component.spec.ts | Unit tests for legacy version mapping and layout defaulting behavior. |
| frontend/src/app/shared/components/fields/display/field-types/single-line-resources-display-field.module.ts | New display field rendering multi-value resources as a full comma-separated single line. |
| frontend/src/app/shared/components/fields/display/field-types/single-line-resources-display-field.module.spec.ts | Unit tests for single-line rendering, title, and empty placeholder behavior. |
| frontend/src/app/shared/components/fields/display/display-field.service.ts | Routes singleline layout + supported multi-value schema types to the new single-line display field. |
| frontend/src/app/shared/components/fields/display/display-field.service.spec.ts | Tests routing logic for singleline/multiline/default layout across supported multi-value schema types. |
| app/models/work_package/exports/macros/attributes.rb | Extends export macro parsing with layout + uses layout to select array separator; maps legacy version. |
Without a boundary the layout alternation matched keyword prefixes, so :multilinefoo applied the multiline layout and left foo as stray text. A word boundary rejects such tokens while trailing punctuation after a valid keyword still parses.
HDinger
left a comment
There was a problem hiding this comment.
Lgtm 👍
I have some general comments:
- I am missing somewhere the information, that the layout argument will not affect all fields.. I can now write something like this
workPackageValue:description:singlelinebut it basically has no effect. Same wih multiline, it only applies to ceratin field types. I admit that I don't know where the best place for this would be. Maybe in the comments explaining the syntax 🤷 - When using
singlelineon a user custom field, the avatars where not shown. Don't know whether this is intendend (or even existing behaviour from the table though)
|
Thanks for the review @HDinger - there's a followup PR updating the docs that will be reviewed with the docs team https://github.com/opf/openproject/pull/24264/changes
I suppose this is ok, no? We can add documentation that user defined layout will take effect only on supported fields.
AFAICT- and testing on QA Edge; avatars were never shown from custom fields- in fact it looks broken, at least testing on Edge. In any case I'm happy to look deeper into it. |
brunopagno
left a comment
There was a problem hiding this comment.
Looks good to me as well. Nice job 👏
The generic single-line field renders plain text, which dropped the avatars user attributes get in every other view. Route []User to a dedicated field that uses the principal renderer's inline mode.
|
Addressed single line avatar rendering in 929cd31
|
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
|
@copilot The following spec(s) are flaky in CI (first seen on PR #24257, linked for reference only):
Treat this as a standalone task, unrelated to PR #24257. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24257 or reuse that branch. Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @akabiru to take over. Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR |









Ticket: https://community.openproject.org/wp/COMMS-876
Multi-value attributes such as target versions render one value per line, which does not always fit the text around a macro. Attribute value macros now accept an optional trailing layout argument (
workPackageValue:X:targetVersions:singlelineor:multiline, multi-line being the default) so authors can choose.data-layoutversionattribute mapped to the single-line behaviourCkEditor render
PDF Export