Skip to content

36597 uve style editor styling non default persona content fails with content not found#36618

Open
dario-daza wants to merge 4 commits into
mainfrom
36597-uve-style-editor-styling-non-default-persona-content-fails-with-content-not-found
Open

36597 uve style editor styling non default persona content fails with content not found#36618
dario-daza wants to merge 4 commits into
mainfrom
36597-uve-style-editor-styling-non-default-persona-content-fails-with-content-not-found

Conversation

@dario-daza

@dario-daza dario-daza commented Jul 17, 2026

Copy link
Copy Markdown
Member

Proposed Changes

  • Fixed a bug in the UVE Style Editor where styling content that was personalized for a specific Persona (i.e. not the "Default Visitor") failed with an error and the change wasn't saved.
  • The editor now correctly keeps track of which Persona you're viewing while you make style changes, so the update is saved against that Persona's version of the content instead of always falling back to the default one.

Additional Info

Previously, saving a style change while viewing a page under a non-default Persona threw an error ("Contentlet not found...") because the save request always looked for the default Persona's version of the content, even when a different Persona was selected in the editor. This fix makes sure the currently selected Persona is included when the style change is saved, so it lands on the correct personalized content.

This PR fixes: #36597

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dario-daza's task in 5m 8s —— View job


Claude finished @dario-daza's task in 2m 54s —— View job


PR Review

I reviewed the backend and frontend changes against origin/main and specifically re-checked the default-visitor regression that @zJaaal flagged earlier.

Resolved

  • PageResourceHelper.java:1207 — The default-visitor dot:persona:dot:persona regression is fixed. resolvePersonalizationFromTag() now guards the bare prefix: UtilMethods.isSet(personaTag) && !Persona.DOT_PERSONA_PREFIX_SCHEME.equals(personaTag), so both null and "dot:persona" resolve to MultiTree.DOT_PERSONALIZATION_DEFAULT. This also protects the saveContent content-add path (line ~786), which shares the same helper.
  • PageResourceTest.java:test_updateStyles_with_defaultVisitor — A default-visitor test was added that sends the bare "dot:persona" and asserts the row lands on dot:default, not dot:persona:dot:persona — locking down exactly the case from the prior review.

New Issues

No blocking issues found. The change is internally consistent:

  • ContentWithStylesForm adds personaTag to the @JsonCreator and correctly excludes it from the @JsonAnySetter contentlet-style capture (ContentWithStylesForm.java:91), so it won't be mistaken for a contentlet ID.
  • reduceStyleForms() correctly extends the dedup key to personaTag|containerId|uuid (PageResource.java:2093, 2112-2114) — style updates for different personas on the same container/uuid no longer collide. MultiKey (Apache Commons) handles the null persona tag fine.
  • Frontend threads $pageData().personaTag into the save payload (dot-uve-style-editor-form.component.ts:483) and the service (dot-page-api.service.ts), matching the backend contract (unprefixed raw keyTag; backend prepends dot:persona:).

Notes (non-blocking)

  • 🟡 The backend guard on "dot:persona" is the robust fix and I'd keep it. Note that the frontend still sends the literal "dot:persona" for the default visitor (DEFAULT_PERSONA.keyTag = 'dot:persona') rather than omitting it — that's fine given the guard, but the @Schema example (returning_visitor) and doc ("Omit for the default persona") slightly understate that the real default value the client sends is "dot:persona". No action required; just worth knowing the guard is load-bearing, not belt-and-suspenders.
  • Test coverage is solid: non-default persona (test_updateStyles_with_nonDefaultPersona), default visitor with bare prefix (test_updateStyles_with_defaultVisitor), plus FE service + component specs. The FE component spec still mocks personaTag: undefined; consider a variant asserting the real "dot:persona" value flows through, but this is covered end-to-end by the backend test.

The prior review's concern is fully addressed and the fix is the correct one (backend guard). LGTM.

@zJaaal

zJaaal commented Jul 17, 2026

Copy link
Copy Markdown
Member

🟠 Possible default-visitor regression — worth verifying before merge

Nice fix for the non-default persona path. One case I'd double-check: the Default Visitor.

The backend converts the tag here (PageResourceHelper.java:207-209):

final String personalization = UtilMethods.isSet(containerEntry.getPersonaTag()) ?
        Persona.DOT_PERSONA_PREFIX_SCHEME + StringPool.COLON + containerEntry.getPersonaTag() : // "dot:persona:" + tag
        MultiTree.DOT_PERSONALIZATION_DEFAULT;                                                   // "dot:default"

For the Default Visitor, $pageData().personaTag is not empty — it's "dot:persona":

  • DEFAULT_PERSONA.keyTag = 'dot:persona' (frontend consts.ts)
  • getDefaultPersona() sets keyTag = DOT_PERSONA_PREFIX_SCHEME = "dot:persona" (PersonaAPIImpl.java:82)

So UtilMethods.isSet("dot:persona") is true, and the personalization resolves to dot:persona:dot:persona instead of dot:default. Before this PR the default case used the hardcoded nulldot:default, so this changes the target for the most common (default) case and could reintroduce the same "not found" symptom — or write styles to a personalization the read path never looks at.

It likely wasn't caught because:

  • the integration test only covers a real non-default persona, and
  • the frontend unit test mocks personaTag: undefined for $pageData, so it never exercises the real "dot:persona" value.

Suggestions (pick one):

  • Frontend (simplest): only send the tag for a real persona — gate on the already-computed isDefaultPersona so the default sends undefined.
  • Backend (most robust): treat the bare prefix as default: UtilMethods.isSet(tag) && !Persona.DOT_PERSONA_PREFIX_SCHEME.equals(tag). This also protects the existing saveContent content-add path, which sends the same "dot:persona" for the default visitor.

Either way, adding a default-visitor test asserting the row lands on dot:default would lock this down.

Side note: the drag-and-drop content flow (utils/index.tspageContainer.personaTag) already sends "dot:persona" for the default through this same backend method. If that flow works today, it's worth checking whether default content is already being stored under dot:persona:dot:persona — that tells you which of the two fixes is the correct one.

Reviewed and drafted by Claude (Claude Code), on behalf of @zJaaal.

@dario-daza

Copy link
Copy Markdown
Member Author

🟠 Possible default-visitor regression — worth verifying before merge

Nice fix for the non-default persona path. One case I'd double-check: the Default Visitor.

The backend converts the tag here (PageResourceHelper.java:207-209):

final String personalization = UtilMethods.isSet(containerEntry.getPersonaTag()) ?
        Persona.DOT_PERSONA_PREFIX_SCHEME + StringPool.COLON + containerEntry.getPersonaTag() : // "dot:persona:" + tag
        MultiTree.DOT_PERSONALIZATION_DEFAULT;                                                   // "dot:default"

For the Default Visitor, $pageData().personaTag is not empty — it's "dot:persona":

  • DEFAULT_PERSONA.keyTag = 'dot:persona' (frontend consts.ts)
  • getDefaultPersona() sets keyTag = DOT_PERSONA_PREFIX_SCHEME = "dot:persona" (PersonaAPIImpl.java:82)

So UtilMethods.isSet("dot:persona") is true, and the personalization resolves to dot:persona:dot:persona instead of dot:default. Before this PR the default case used the hardcoded nulldot:default, so this changes the target for the most common (default) case and could reintroduce the same "not found" symptom — or write styles to a personalization the read path never looks at.

It likely wasn't caught because:

  • the integration test only covers a real non-default persona, and
  • the frontend unit test mocks personaTag: undefined for $pageData, so it never exercises the real "dot:persona" value.

Suggestions (pick one):

  • Frontend (simplest): only send the tag for a real persona — gate on the already-computed isDefaultPersona so the default sends undefined.
  • Backend (most robust): treat the bare prefix as default: UtilMethods.isSet(tag) && !Persona.DOT_PERSONA_PREFIX_SCHEME.equals(tag). This also protects the existing saveContent content-add path, which sends the same "dot:persona" for the default visitor.

Either way, adding a default-visitor test asserting the row lands on dot:default would lock this down.

Side note: the drag-and-drop content flow (utils/index.tspageContainer.personaTag) already sends "dot:persona" for the default through this same backend method. If that flow works today, it's worth checking whether default content is already being stored under dot:persona:dot:persona — that tells you which of the two fixes is the correct one.

Reviewed and drafted by Claude (Claude Code), on behalf of @zJaaal.

Good catch, and worth double-checking carefully given how easy it'd be to reintroduce the exact bug we just fixed. I traced through the actual render path to see whether $pageData().personaTag really can be "dot:persona" for the Default Visitor:

  • VisitorAPIImpl.java — when com.dotmarketing.persona.id is sent (including the literal modes.persona.no.persona sentinel the frontend uses for "Default Visitor"), it tries personaAPI.find(...), which does findContentletByIdentifier("modes.persona.no.persona", ...). That's not a real contentlet ID, so it throws — caught explicitly (catch(DotContentletStateException e), comment: "meant to catch the 'Can't find contentlet' error") — and falls through to visitor.setPersona(null).
  • ViewAsPageStatus.persona is set from visitor.getPersona() → null in this case.
  • ViewAsPageStatusSerializer.java — when persona is null, the "persona" key is never added to the response JSON at all — not null, structurally absent. Same for GraphQL (PageAPIGraphQLTypesProvider pulls persona off the same ViewAsPageStatus via PropertyDataFetcher).
  • PersonaAPIImpl.getDefaultPersona() — the method that produces keyTag = "dot:persona" — is only called from getPersonas() (the dropdown-list builder), never from the visitor/render path that populates viewAs.

So viewAs.persona is genuinely absent for both "no persona param sent" and "explicitly selected Default Visitor," which means $pageData().personaTag is undefined in practice, not "dot:persona".

That said, the underlying worry is legitimate as a robustness concern, personaTag is client-supplied on a public REST endpoint, and if this assumption were ever wrong (a future change, a different client, a bug in persona resolution) the effect would be a silently-wrong personalization key rather than a loud error. So we added the guard anyway rather than relying on "we checked, it can't happen today": PageResourceHelper.resolvePersonalizationFromTag() now treats a bare "dot:persona" (no tag segment) as default, same as a missing tag. It's a one-line change and it also covers the pre-existing addContent()/drag-and-drop save path, which goes through the same logic and reads the same $pageData().personaTag.

Also added test_updateStyles_with_defaultVisitor to lock down the contract — asserts both null and the bare "dot:persona" prefix resolve to dot:default, so this can't silently regress.

@mergify

mergify Bot commented Jul 17, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

UVE Style Editor: styling non-default Persona content fails with HTTP 400 CONTENT_NOT_FOUND

2 participants