-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(core): update getPublishDateFromRelease, add ScheduledRelease type #7754
base: corel
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,23 @@ export interface ReleaseSystemDocument { | |
state: ReleaseState | ||
publishAt?: string | ||
} | ||
|
||
/** | ||
* @internal | ||
*/ | ||
export interface ScheduledRelease extends ReleaseDocument { | ||
publishAt?: string | ||
metadata: ReleaseDocument['metadata'] & { | ||
intendedPublishAt: string | ||
releaseType: 'scheduled' | ||
} | ||
} | ||
/** | ||
* @internal | ||
*/ | ||
export const isScheduledRelease = (release: ReleaseDocument): release is ScheduledRelease => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, the naming here is vague to me, because a scheduled release to me has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think you are right, we need to find a better name for the intended schedule releases |
||
release.metadata.releaseType === 'scheduled' && 'intendedPublishAt' in release.metadata | ||
|
||
/** | ||
* @internal | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ import { | |
} from '../../util' | ||
import {type CurrentPerspective} from '../hooks/usePerspective' | ||
import {type VersionOriginTypes} from '../index' | ||
import {type ReleaseDocument} from '../store/types' | ||
import {type ReleaseDocument, type ScheduledRelease} from '../store/types' | ||
import {LATEST} from './const' | ||
|
||
/** | ||
|
@@ -64,28 +64,12 @@ export function getCreateVersionOrigin(documentId: string): VersionOriginTypes { | |
} | ||
|
||
/** @internal */ | ||
export function getPublishDateFromRelease(release: ReleaseDocument): Date { | ||
export function getPublishDateFromRelease(release: ScheduledRelease): Date { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the type to |
||
const dateString = release.publishAt || release.metadata.intendedPublishAt | ||
if (!dateString) { | ||
console.error('No publish date found on release', release) | ||
return new Date() | ||
} | ||
|
||
return new Date(dateString) | ||
} | ||
|
||
/** @internal */ | ||
export function getReleasePublishDate( | ||
release: Pick<ReleaseDocument, 'publishAt'> & { | ||
metadata: Pick<ReleaseDocument['metadata'], 'intendedPublishAt'> | ||
}, | ||
): Date | null { | ||
const publishDate = release.metadata.intendedPublishAt || release.publishAt | ||
|
||
if (!publishDate) return null | ||
return new Date(publishDate) | ||
} | ||
|
||
/** @internal */ | ||
export function isPublishedPerspective(bundle: CurrentPerspective | string): bundle is 'published' { | ||
return bundle === 'published' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't a like for like swap I don't think, since
isScheduledRelease
only looks atintendedPublishAt
. I can't recall exactly but it might be worth verifying that when a scheduled release is actually scheduled, and it's possible to reconfirm the scheduled datetime, whether we also update theintendedPublishAt
to reflex this too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we removing the
intendedPublishAt
when scheduling it?It's confusing how close this names are.
Maybe we need to find a better name for the
releaseType ="scheduled"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we leave
intendedPublishAt
as whatever it was, and only persist this new date intopublishAt
via the schedule action. We should agree either to clear it or always keep it aligned. But of course the schedule action can also be triggered outwith studio, so guaranteeing they match isn't possible