[Content]: Schedule Unpublish Button - #4028
Conversation
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
|
@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 — 🔴 1 blocker(s) · 🟡 1 warning(s) — see inline comments |
Code Review — ✅ No blockers · 🟡 2 warning(s) — see inline comments |
Code Review — ✅ No blockers |
Code Review — ✅ No blockers |
|
@geodem127 please run ci on this |
Code Review — ✅ No blockers · 🟡 2 warning(s) — see inline comments |
Code Review — ✅ No blockers · 🟡 2 warning(s) — see inline comments |
Code Review — ✅ No blockers · 🟡 1 warning(s) — see inline comments |
Code Review — ✅ No blockers |
Code Review — ✅ No blockers · 🟡 1 warning(s) — see inline comments |
| placement="bottom-start" | ||
| > | ||
| <Stack | ||
| data-cy="ScheduledUnpublishIndicator" |
There was a problem hiding this comment.
🟡 Duplicate data-cy="ScheduledUnpublishIndicator" rendered simultaneously by two components with independent state
ItemEditHeaderActions.tsx (line ~651) also puts data-cy="ScheduledUnpublishIndicator" on its "Published" label whenever its own hasScheduledUnpublish (derived from the legacy state.content[itemZUID].publishing slice) is true. Both components are siblings under the same ItemEditHeader (index.tsx:247 and :255), so when viewing the currently-published version — the common case, since this block has no !== currentVersion guard unlike the sibling activePublishing/scheduledPublishing blocks above it — two DOM nodes will match this selector at once, sourced from different queries (RTK useGetItemPublishingsQuery here vs. legacy Redux state there) that can go out of sync after schedule/unschedule actions. The new Cypress spec asserts cy.getBySelector("ScheduledUnpublishIndicator").should("not.exist") after unscheduling — if one of the two sources lags, that assertion is a flake risk in addition to violating the project's one-selector-one-target convention. Use a distinct data-cy here (e.g. PublishStatusScheduledUnpublishIndicator) or derive both from the same data source.
Code Review — ✅ No blockers · 🟡 1 warning(s) — see inline comments |
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