Conversation
geodem127
changed the title
4c2e3e7 to
6609be8
Compare
Coverage ReportOverall Coverage
Changed Files Coverage
|
QA — Gaps & concerns + suggested Cypress coverageVerified the core Schedule Unpublish flow end-to-end against a live published item (open menu → Schedule Unpublish → set date → schedule → indicator shows → Unschedule → state restored). The
Gaps & concerns
Suggested Cypress coverageThe new cases in
|
Review summarySolid extension to the existing A few issues to address before merge — inline comments below for specifics. Bugs
Test concerns
Behavior question
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
@geodem127 please state the changes that have been introduced due to this review |
|
@agalin920 — addressed in 64058ee. Here's what was changed: Gaps & concerns:
Cypress coverage additions:
Both new tests are now fully self-contained (`cy.visit` + `cy.wait`); the "Cancels" test schedules an unpublish before cancelling it. |
Code Review — ✅ No blockers · 🟡 1 warning(s) — see inline comments |
# Conflicts: # src/apps/content-editor/src/app/views/ItemEdit/components/ItemEditHeader/ItemEditHeaderActions.tsx # src/apps/content-editor/src/app/views/ItemEdit/components/ItemEditHeader/PublishStatus.tsx # src/shell/components/SchedulePublish/index.tsx # src/shell/store/content.js
f03a5ba
Localization Reviewer — ✅ No blockers · 🟡 1 advisory note(s)🔴 BlockingNone 🟡 Advisory
|
| onUnscheduleSuccess={() => { | ||
| if (publishAfterUnschedule) { | ||
| setIsConfirmPublishModalOpen(true); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🔴 Stale publishAfterUnschedule flag leaks into the new unschedule-unpublish flow, popping an unwanted publish confirmation
onUnscheduleSuccess is now shared by both the "unschedule publish" flow and the brand-new "unschedule unpublish" flow, but publishAfterUnschedule is only ever set from the publish flow (line 811) and only ever cleared inside ConfirmPublishModal's cancel/confirm handlers (lines 856/860). If a user opens "Publish Now" while scheduled (setting publishAfterUnschedule=true) and then just closes that dialog via onClose instead of confirming, the flag stays true forever. The next time they cancel a scheduled unpublish on an already-published item, this stale flag fires setIsConfirmPublishModalOpen(true), surfacing an unrelated "confirm publish" modal — and confirming it triggers an unintended handlePublish(). Reset publishAfterUnschedule in the SchedulePublish/dialog onClose handler, or gate this check on scheduledAction === "publish".
There was a problem hiding this comment.
Fixed — gated the onUnscheduleSuccess check on scheduledAction === "publish" in addition to publishAfterUnschedule, so a stale flag from an abandoned publish flow can no longer trigger the publish-confirm modal during an unpublish-unschedule flow.
| placement="bottom-start" | ||
| > | ||
| <Stack | ||
| data-cy="ScheduledUnpublishIndicator" |
There was a problem hiding this comment.
🟡 Duplicate data-cy="ScheduledUnpublishIndicator" renders simultaneously with ItemEditHeaderActions.tsx
ItemEditHeaderActions.tsx (line 663-667) already tags its "Published" label with data-cy="ScheduledUnpublishIndicator" whenever hasScheduledUnpublish is true, and that component mounts alongside this PublishStatus in the same header (ItemEditHeader/index.tsx). Since both conditions describe the same real-world state (published item with a future unpublishAt), the page ends up with two elements sharing this data-cy, breaking the single-selector-per-target convention this repo relies on for Cypress. Give this badge a distinct data-cy value.
Code Review — 🔴 1 blocker · 🟡 1 warning(s) — see inline comments |
Prevents a leftover flag from an abandoned "publish now while scheduled" flow from popping the publish-confirm modal during an unrelated unschedule-unpublish flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Localization Reviewer — 🔴 10 blocker(s) — see inline comments🔴 Blocking
🟡 AdvisoryNone |
| "booleanHide": "Ocultar", | ||
| "booleanShow": "Mostrar", | ||
| "browse": "Explorar", | ||
| "cancelledScheduledUnpublish": "Cancelled scheduled unpublish for {{title}}", |
There was a problem hiding this comment.
Localization Reviewer: new keys (cancelledScheduledUnpublish, errorCancellingScheduledUnpublish, errorSchedulingUnpublish, itemEditTooltipPublished, itemEditTooltipSaved, itemEditTooltipScheduledToPublish, itemEditTooltipScheduledToUnpublish, itemEditUnscheduleUnpublish, itemEditVersionScheduledUnpublishLabel, scheduledUnpublish) are copied verbatim from English, not translated to Spanish.
| "booleanHide": "छिपाएं", | ||
| "booleanShow": "दिखाएं", | ||
| "browse": "ब्राउज़ करें", | ||
| "cancelledScheduledUnpublish": "Cancelled scheduled unpublish for {{title}}", |
There was a problem hiding this comment.
Localization Reviewer: same set of new keys copied verbatim from English, not translated to Hindi.
| "booleanHide": "隐藏", | ||
| "booleanShow": "显示", | ||
| "browse": "浏览", | ||
| "cancelledScheduledUnpublish": "Cancelled scheduled unpublish for {{title}}", |
There was a problem hiding this comment.
Localization Reviewer: same set of new keys copied verbatim from English, not translated to Chinese.
| "booleanHide": "Скрыть", | ||
| "booleanShow": "Показать", | ||
| "browse": "Обзор", | ||
| "cancelledScheduledUnpublish": "Cancelled scheduled unpublish for {{title}}", |
There was a problem hiding this comment.
Localization Reviewer: same set of new keys copied verbatim from English, not translated to Russian.
| "booleanHide": "Verbergen", | ||
| "booleanShow": "Weergeven", | ||
| "browse": "Bladeren", | ||
| "cancelledScheduledUnpublish": "Cancelled scheduled unpublish for {{title}}", |
There was a problem hiding this comment.
Localization Reviewer: same set of new keys copied verbatim from English, not translated to Dutch.
| "schedulePublishTitleUnschedule": "Desprogramar publicación", | ||
| "schedulePublishUnscheduleButton": "Desprogramar publicación", | ||
| "schedulePublishUnscheduleInfo": "Esto habilitará la posibilidad de programar o publicar otras versiones de este elemento de contenido", | ||
| "scheduleUnpublishButton": "Schedule Unpublish", |
There was a problem hiding this comment.
Localization Reviewer: 8 new scheduleUnpublish* keys are copied verbatim from English, not translated to Spanish.
| "schedulePublishTitleUnschedule": "प्रकाशन अनिर्धारित करें", | ||
| "schedulePublishUnscheduleButton": "प्रकाशन अनिर्धारित करें", | ||
| "schedulePublishUnscheduleInfo": "इससे इस कंटेंट आइटम के अन्य संस्करणों को निर्धारित या प्रकाशित करने की क्षमता सक्षम हो जाएगी", | ||
| "scheduleUnpublishButton": "Schedule Unpublish", |
There was a problem hiding this comment.
Localization Reviewer: same 8 new scheduleUnpublish* keys copied verbatim from English, not translated to Hindi.
| "schedulePublishTitleUnschedule": "取消定时发布", | ||
| "schedulePublishUnscheduleButton": "取消定时发布", | ||
| "schedulePublishUnscheduleInfo": "这将允许定时或发布此内容项的其他版本", | ||
| "scheduleUnpublishButton": "Schedule Unpublish", |
There was a problem hiding this comment.
Localization Reviewer: same 8 new scheduleUnpublish* keys copied verbatim from English, not translated to Chinese.
| "schedulePublishTitleUnschedule": "Отменить запланированную публикацию", | ||
| "schedulePublishUnscheduleButton": "Отменить публикацию", | ||
| "schedulePublishUnscheduleInfo": "Это позволит планировать или публиковать другие версии этого элемента контента", | ||
| "scheduleUnpublishButton": "Schedule Unpublish", |
There was a problem hiding this comment.
Localization Reviewer: same 8 new scheduleUnpublish* keys copied verbatim from English, not translated to Russian.
| "schedulePublishTitleUnschedule": "Publicatie annuleren", | ||
| "schedulePublishUnscheduleButton": "Publicatie annuleren", | ||
| "schedulePublishUnscheduleInfo": "Hiermee kun je andere versies van dit content-item plannen of publiceren", | ||
| "scheduleUnpublishButton": "Schedule Unpublish", |
There was a problem hiding this comment.
Localization Reviewer: same 8 new scheduleUnpublish* keys copied verbatim from English, not translated to Dutch.
| }} | ||
| onUnscheduleSuccess={() => { | ||
| if (publishAfterUnschedule) { | ||
| if (publishAfterUnschedule && scheduledAction === "publish") { |
There was a problem hiding this comment.
🔴 "Publish Now" on a scheduled item no longer opens the confirm-publish modal after unscheduling
onUnscheduleSuccess now requires scheduledAction === "publish", but the path that reaches it via the top-level menu action (setPublishAfterUnschedule at line 809-812, wired to the ITEM_STATES.scheduled case of the main publish MenuItem) opens the dialog via setScheduledPublishDialogOpen(true) directly — it never calls setScheduledAction("publish"). scheduledAction stays at its previous value (null in the common case), so after unscheduling, setIsConfirmPublishModalOpen(true) never fires and the user is left with an unscheduled, unpublished item with no prompt to publish it. Either call setScheduledAction("publish") in the setPublishAfterUnschedule handler, or drop the scheduledAction === "publish" condition here.
| {isAlreadyScheduled ? ( | ||
| <> | ||
| <Alert severity="info" icon={<InfoRoundedIcon />}> | ||
| {t("shell.schedulePublishUnscheduleInfo")} |
There was a problem hiding this comment.
🟡 Wrong copy shown in the "unschedule unpublish" dialog
When isAlreadyScheduled is true this reuses shell.schedulePublishUnscheduleInfo ("This will enable the ability to schedule or publish other versions of this content item"), which describes cancelling a scheduled publish, not a scheduled unpublish. A dedicated key (e.g. a new scheduleUnpublishUnscheduleInfo) describing that cancelling keeps the item published should be used instead — this dialog already has unused, more accurate copy defined (scheduleUnpublishScheduledInfo) but not this info-alert text.
Code Review — 🔴 1 blocker(s) · 🟡 1 warning(s) — see inline comments |
- Fix regression from the earlier publishAfterUnschedule fix: set scheduledAction to "publish" when opening Publish Now from a scheduled item, so the confirm-publish modal fires correctly after unscheduling. - Give PublishStatus's scheduled-unpublish badge its own data-cy value to stop it colliding with ItemEditHeaderActions's. - Add a dedicated shell.scheduleUnpublishUnscheduleInfo copy key instead of reusing the scheduled-publish dialog's info text. - Translate the PR's new content.json/shell.json keys across es-ES, hi-IN, zh-CN, ru-RU, nl-NL. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Localization Reviewer — ✅ No blockers · 🟡 1 advisory note(s)🔴 BlockingNone 🟡 Advisory
|
| userName={getUserNameByZUID( | ||
| lastItemUpdateAudit?.actionByUserZUID || | ||
| item?.web?.createdByUserZUID | ||
| )} |
There was a problem hiding this comment.
🟡 "Saved by" tooltip can go blank for users no longer in the org
The old code fell back to lastItemUpdateAudit?.firstName/lastName (embedded on the audit record at the time of the action) before doing a ZUID lookup against the live users list. This version drops that fallback and only resolves the name via getUserNameByZUID, so if the user who made the edit was later removed/deactivated and isn't in the current users query result, the tooltip silently shows no name instead of the historically-correct one from the audit record.
Code Review — ✅ No blockers · 🟡 1 warning(s) — see inline comments |
3 participants
What
Adds Schedule Unpublish and Cancel Scheduled Unpublish flows to the content item editor.
Changes
New functionality
ItemEditHeaderActions(published state only): "Schedule Unpublish" / "Unschedule Unpublish"SchedulePublishnow acceptsscheduledAction="unpublish"prop and routes to a dedicatedScheduleUnpublishDialogcomponentScheduledUnpublishIndicatorbadge on the published label (data-cy="ScheduledUnpublishIndicator")PublishStatuscomponent shows scheduled-unpublish badge when the active version has a futureunpublishAtArchitecture
SchedulePublishrefactored into a smart container (index.tsx) and two dumb presentation components:SchedulePublishDialog— publish scheduling flow (exact pre-PR behaviour)ScheduleUnpublishDialog— unpublish scheduling flow (new)TooltipTitlenow takes a resolveduserNameprop instead ofuserZUID+ the fulluserslist; each caller (ItemEditHeaderActions,PublishStatus) resolves the name itselfBug fixes
unpublish()thunk now re-throws in.catch()sohandleUnscheduleUnpublishstep-2 is correctly gated on step-1 successhandleSchedulePublish/handleScheduleUnpublish:onScheduleSuccessmoved from.finally()to.then()so it only fires on success, not on API errorhandleUnschedulePublish:.catch()added to prevent unhandled promise rejection leaking to Sentry (error notification is already handled by the thunk)handleSchedulePublish/handleScheduleUnpublish:.catch()added for the same reasonhasScheduledUnpublishwrapped in!!()so its type isbooleanrather thanstring | boolean | undefinedAudittype imported inItemEditHeaderActionsto satisfynoImplicitAnyunpublishAt: "never"(avoids API "already has scheduled publish event" error)publish()thunk notification correctly shows "Cancelled scheduled unpublish for …" for thepublishAt: "now" / unpublishAt: "never"path (success and error branches)handleUnscheduleUnpublish: fixedthrow new Error(response?.error?.message)—publish()/unpublish()resolve their.catch()to{ error: message }wheremessageis a plain string, so.messagewas alwaysundefined. Now throwsresponse?.errordirectly so the real error text is preserved.publish()thunk.catch(): added the missing branch for a failedhandleScheduleUnpublishcall (publishAt: "now"+ a futureunpublishAt). It previously fell through to the generic "Error publishing …" toast instead of an unpublish-specific one.hasScheduledUnpublish(ItemEditHeaderActions),isAlreadyScheduledUnpublish(SchedulePublish), and thePublishStatusbadge all gated visibility on the currently loaded/edited version matching the published version's version number. Saving a new draft bumpsmeta.version, so the badge and "Unschedule Unpublish" menu label disappeared even though the scheduled unpublish was still armed on the published version, with no way to cancel it from the UI. All three now gate onitem.publishing.isPublished, which reflects the published version's own state independent of what's currently loaded/edited.handleScheduleUnpublishnow mirrors thehasAnyScheduledPublishpre-deletion guard already used byhandleUnscheduleUnpublish: it deletes a conflicting scheduled publish before POSTing the newunpublishAtschedule, instead of letting the API reject it with "already has a scheduled publish event."ScheduleUnpublishDialogalready warns the user that scheduling an unpublish cancels a pending scheduled publish — this makes that promise actually hold. Flagged in review.Cypress
cy.visit()+cy.wait([items, publishings])Screen.Recording.2026-03-26.at.2.20.33.AM.mov