Skip to content
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

SOF-1926 Update AdLibActions and -Pieces rank #267

Conversation

LindvedKrvang
Copy link

Ranks on AdLibActions and -Pieces are also updated when the Segment ranks are being updated

)

for (const adLibAction of adLibActionsForSegment) {
const oldRankFraction: number = (adLibAction.display._rank ?? 1) % 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why ?? 1 instead of ?? 0? I assume that 0 is the intended default value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 was intentional, but mostly to ensure that he rank did have a decimal value.
I don't think that's a strong argument so I have updated it to be 0.

@@ -432,6 +437,36 @@ export async function handleUpdatedSegmentRanks(
)
}

function updateAdLibActionRanks(segmentExternalId: string, newRank: number, cache: CacheForIngest): void {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice for clarification that newRank is the newSegmentRank.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence, but I have updated it.

@LindvedKrvang LindvedKrvang merged commit 092546c into staging Apr 29, 2024
45 checks passed
@LindvedKrvang LindvedKrvang deleted the SOF-1926/updateRanksOnAdLibActionsAndPiecesOnSegmentRankUpdated branch April 29, 2024 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants