Fix CMS 13 / Optimizely Identity picker & editor issues (#56) - #57
Hidden character warning
Conversation
mariajemaria
left a comment
There was a problem hiding this comment.
Code review — 1 substantive gap (search is not filtered by [AllowedTypes]) plus a few minor/cosmetic notes, left inline. Nice, well-tested fix set overall; strengths posted as a separate comment.
👍 What's strong in this PR
Clear, issue-referenced comments throughout make the intent of each change easy to follow. 🙌 |
|
mariajemaria
left a comment
There was a problem hiding this comment.
Approve with comments
Well-scoped, cohesive fix for the #56 integration issues on CMS 13 / OptimizelyIdentity. Excellent commenting discipline (each block cites its issue point and the why) and real server-side test coverage. Nothing blocking below — two items worth a follow-up.
Worth addressing
- Client ignores the new
Publishedflag.categoryApi.createreturns only{ id, name }and the picker optimistically selects the new category. When the server saved a draft (required-fields fallback, point 5), the user ends up referencing an incomplete unpublished draft with no cue. The server plumbing (published) is in place and tested but currently unused on the client — consider surfacing a "saved as draft, complete required fields" hint. ResolveShellBasePathassumes the client-resource base == shell-stores base. Fix #3 derives the base fromPaths.ToClientResource(...)and reuses it forcms/Stores/…andshell/Stores/…. Correct for default CMS and (per the issue)/ui/under OptimizelyIdentity, but the module client-resource root and the shell-store root are conceptually distinct. A comment recording this as an observed assumption (link the discovery notes) would help future readers.
Minor
Selectableis an N+1 (TryGetper id). Usually small batches, butIContentRepository.GetItems(...)would collapse it into one call.- Search can trap an already-selected, now-non-selectable category —
applySearchSelectabilitydisables the checkbox unconditionally, unlike the tree path which notes existing selections are removed via chips. Confirm this is intended. - Assembly dedup is guarded in code and tested for the
AddApplicationPartcase;module.configstill lists<assemblies>. Confirm the module-loader path produces anAssemblyPartwith the sameAssemblyinstance so thepart.Assembly == assemblycheck can't miss and double-register the route. - Async selectability race: a user can tick a non-selectable box in the brief window before
getSelectableresolves; it then becomes disabled but stays checked. Low impact.
Security
Authorization preserved and improved — class-level policy covers both Create and Selectable, same roles, and scheme pinning fixes the 500 when no default challenge scheme exists. SkipValidation is scoped to unpublished Save, so invalid content can't go live. Good.



Issues reported here #56