[OP-19358] Show popover values verbatim - #25123
Open
myabc wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The functional change is well-scoped and backed by targeted tests; remaining feedback is a small import hygiene improvement.
Pull request overview
Fixes incorrect rendering of non-ASCII strings in the “My spent time” popover by avoiding HTML sanitization that serialized characters into numeric references, and by moving row construction into a pure helper with focused unit coverage.
Changes:
- Remove
DomSanitizer.sanitize(SecurityContext.HTML, …)from popover value handling so lit text bindings render verbatim characters. - Extract popover row construction into
timeEntryPopoverRows()as a pure function. - Add specs covering Cyrillic values and verifying values are bound as text (not markup).
File summaries
| File | Description |
|---|---|
| frontend/src/app/features/calendar/te-calendar/te-calendar.component.ts | Stops sanitizing popover values and delegates row creation to a helper. |
| frontend/src/app/features/calendar/te-calendar/te-calendar-popover.ts | Introduces TimeEntrySchema/timeEntryPopoverRows() and keeps popover template text-bound. |
| frontend/src/app/features/calendar/te-calendar/te-calendar-popover.spec.ts | Adds tests for verbatim non-ASCII output and “no markup rendering” behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
myabc
force-pushed
the
bug/op-19358-popover-values-verbatim
branch
from
September 3, 2026 19:43
1c38d73 to
864da05
Compare
myabc
marked this pull request as ready for review
September 3, 2026 19:43
3 tasks
Angular's HTML sanitizer encodes every non-ASCII character as a numeric character reference, and lit binds the result as text, so Cyrillic values showed up as "Т…". Drops the sanitizer: lit's text binding already escapes, so no HTML was ever rendered from these strings. Moves row building next to the template so it can be unit-tested. https://community.openproject.org/wp/OP-19358
myabc
force-pushed
the
bug/op-19358-popover-values-verbatim
branch
from
September 4, 2026 08:32
864da05 to
6c9c042
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. |
oliverguenther
approved these changes
Sep 7, 2026
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
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.
Note
Stacked on #25112 (DREAM-813). Only the last commit belongs to this PR.
Ticket
https://community.openproject.org/wp/OP-19358
What are you trying to accomplish?
The "My spent time" popover showed non-ASCII values as numeric character references (
Ша…) while the labels and the entry block behind it rendered the same text correctly.Every value went through
DomSanitizer.sanitize(SecurityContext.HTML, …), which serialises non-ASCII characters as&#NNNN;. lit then bound the result as text, so the references were displayed literally.Screenshots
What approach did you choose and why?
Drops the sanitizer: lit's text binding already escapes, so no HTML was ever rendered from these strings. Row building moves out of the component into
timeEntryPopoverRows()next to the lit template, so it is a pure function with a spec covering Cyrillic values and a comment containing<b>staying a literal string.Merge checklist